fix sigmatch: can't infer constrained generic params#25871
Conversation
|
this looks like a chronos bug. The test is meant to attempt matching |
| f: PType): PType = | ||
| result = lookup(m.bindings, f) | ||
| if result == nil: | ||
| let oldErrorCounter = c.config.errorCounter |
There was a problem hiding this comment.
Why does it try to instantiate a generic with an unsufficient number of inferred typevars? This fix doesn't attack the root cause.
Graveflo
force-pushed
the
ovrl_inst_nil
branch
2 times, most recently
from
a07a71b to
35b4b54
Compare
| internalError(c.graph.config, arg.info, "getInstantiatedType") | ||
| result = errorType(c) | ||
| if allParamsBound(m.bindings, f): | ||
| result = generateTypeInstance(c, m.bindings, arg, f) |
There was a problem hiding this comment.
Instead make generateTypeInstance return nil on missing type-variables and don't produce an internalError for it.
There was a problem hiding this comment.
this would require digging all the way into lookupTypeVar and replaceTypeVarsTAux which can be mitigated with a flag, but also semAfterMacroCall inferWithMetatype and instantiateGenericParamList use generateTypeInstance as well and would have their error behavior changed. In order for generateTypeInstance to return nil instead of erroring the error paths have to be disabled for all of these calls which either loses granularity on which error happened, forces a second error-enabled path like sigmatch does, or just generally makes it confusing what to return and what side effects to expect since the error paths got disabled.
I can understand not wanting to do the pre-pass if it can be avoided, but I'm not sure how to do that cleanly. Maybe I'm just on the wrong track?
(edit: wait, I can just make generateTypeInstance take a boolean... let me try that)
| var m = newCandidate(c, f) | ||
| result = typeRel(m, f, a) | ||
|
|
||
| proc allParamsBound(bindings: LayeredIdTable, t: PType): bool = |
There was a problem hiding this comment.
Should fall out naturally for a properly coded generateTypeInstance.
|
pretty sure this is a runner error and not the patch. Looks like there are 3 (maybe 4?) methods for fixing this:
|
2 participants
test self-explanatory