[compiler-v2] Avoid infinite recursion showing types with receiver functions in presence of errors #14922
Conversation
⏱️ 2h 25m total CI duration on this PR
|
brmataptos
requested review from
wrwg,
vineethk,
rahxephon89,
fEst1ck and
igor-aptos
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14922 +/- ##
=======================================
Coverage 60.1% 60.1%
=======================================
Files 856 856
Lines 211110 211138 +28
=======================================
+ Hits 126962 127011 +49
+ Misses 84148 84127 -21 ☔ View full report in Codecov by Sentry. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| public fun borrow<K, V>(self: &OrderedMao<K, V>, key: &K): &V { |
There was a problem hiding this comment.
There appears to be a typo in the type name OrderedMao. This should be corrected to OrderedMap to match the struct definition and maintain consistency throughout the code.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Well, in this case it's in a test directory, so I intended it to be incorrect, and it should not be corrected. Their bot should notice a directory named compiler.*test.* and consider the possibility that the code is test input code for a compiler.
| 39 │ public fun borrow<K, V>(self: &OrderedMao<K, V>, key: &K): &V { | ||
| │ ^^^^^^^^^^ | ||
|
|
||
| error: unable to infer instantiation of type `fun self.iter_borrow(Self,&*error*):&V` (consider providing type arguments or annotating the type) |
There was a problem hiding this comment.
The type fun self.iter_borrow(Self,&*error*):&V mentioned here is not particularly helpful to the user. Maybe we could display something better when *error* is involved?
Could be addressed in a separate PR though.
There was a problem hiding this comment.
The *error* shown here is the type K in the parameter type. What we probably should do is to not print this error message, since we already see an*error* type and we presumably print another error message when generating that type.
wrwg
changed the title
| @@ -3264,6 +3264,8 @@ | |||
| pub used_modules: BTreeSet<ModuleId>, | |||
| /// Whether to use `m::T` for representing types, for stable output in docgen | |||
| pub use_module_qualification: bool, | |||
| /// Var type that should appear as Self in display | |||
| pub opt_self_var: Option<u32>, | |||
There was a problem hiding this comment.
I don't understand this. Self does not play a special rule, and a variable specifically shouldn't be mapped to anything as long as it is not assigned. This does not seem to be the right fix. What is the problem? Is it a cyclic substitution? Then this is a bug around a cycle check missing in type unification.
There was a problem hiding this comment.
I've added a comment to the issue to carefully explain the problem, which is that
- To display a variable type constrained by just a
SomeReceiverFunctionconstraint, we display the signature of the function, whose first parameter type is the variable type itself, so we try to display it...
Self does play a special role, in that we may have a type that is unconstrained other than by being the target of a receiver function call. This happens only in the case of an error, but that's why we are displaying the type.
Possible solutions include:
- Catching the error earlier and avoiding any attempts to print out the
SomeReceverFunctionconstraint. - Always displaying the first parameter type of
SomeReceiverFunctionasSelf, ignoring the actual type.- This seems like it could lose info in some other cases, where the first type is displayable without this loop
- Adding a hack to show
Selfin this particular case. - Generalize this hack to be able to print recursive types if they happen to show up: Add new substitution for the variable, and if the substitution is used, then display an explicitly recursive type such as
exists v. fun self.method(v, ...)
I was able to get rid of one occurrence of the error earlier, but one remained, so I went to solution #3. I alluded to the possible desire to generalize to solution #4 in the PR summary, but I think it's overkill for the current situation.
There was a problem hiding this comment.
I've changed it to use .. instead of Self, and also to allow multiple variables to be stored when displaying via constraints to catch potential recursions. I also check the error type name for *error* in the one case where it is showing up, and avoid showing the error at all in this case, since it is guaranteed to be redundant. This has the unfortunate side-effect of leaving us without any test case to show recursion happening, but it was tested before this change.
…s. check for `*error*` in type for instantiation type error message, avoid showing an error message which is actually redundant
brmataptos
force-pushed
the
brm-issue-14913
branch
from
2464f91 to
0f45016
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
When displaying a "receiver function" type in the presence of errors,
a type variable may be shown by showing the receiver function type,
which includes the type variable itself. We break the recursion by
introducing an
opt_self_varfield inTypeDisplayContextwhichis used to recognize the type variable and display "Self" instead
of recursing.
Fixes #14913
How Has This Been Tested?
Added a reduced test based on Igor's finding.
Key Areas to Review
If we allow more recursive types/constraint in the future then we
may want a more general mechanism. I think we should punt for now,
but others may disagree.
Type of Change
Which Components or Systems Does This Change Impact?
Checklist