Fix #2412: Local error causes spurious errors to be reported #2922
Conversation
smarter
force-pushed
the
fix-spurious-errors
branch
from
1fef282 to
66801cc
Compare
|
Ping for review. |
There was a problem hiding this comment.
It would be good to motivate this in more detail, because I am not convinced yet. Logically, it seems correct to store the context in the prototype and evaluate arguments in this context. You compensate for supercall arguments by passing a special flag. But how do you know architecturally that you do not need other compensations as well?
| /** Evalute `op` with the proper context to type the arguments. */ | ||
| protected def withArgCtx[T](op: Context => T)(implicit ctx: Context) = { | ||
| if (isSelfConstrCall && !thisCallArgCtxUsed) { | ||
| // Context#thisCallArgContext is not idempotent so we must only use it |
There was a problem hiding this comment.
Can we use Mode.InSuperCall as an indicator whether we are already in the right context, instead of defining a special field for this?
There was a problem hiding this comment.
Yes that should work, I'll give it a try.
That's fine as long as there is exactly one way to type the arguments and no backtracking takes place, but there's plenty of backtracking in
Yes, it's a bit dicey, it relies on the fact that |
In several situations, instead of the common pattern in the compiler of using the enclosing Context available coming an implicit method parameter, we used some other Context. So far, this wasn't an issue but starting with the next commit it is important for FunProto#typedArgs to be called in a Context whose owner chain contains the Context where the FunProto was created. This motivated the following changes: - RunInfo does not actually need to keep a reference to the root Context: It was previously used by `ImplicitRunInfo#implicitScope`, but can be replaced by using the Context from the enclosing scope (which was already used in this method with the name `liftingCtx`). - OfTypeImplicits does not need to keep a reference to the root Context either, we can just replace its lazy vals by defs that cache their results. - ContextualImplicits does need to keep track of the Context from which its implicit references come from, but this Context itself does not have to be available implicitly in the class, instead the Context from the enclosing scope is used. This also requires replacing OfTypeImplicits#toString and ContextualImplicits#toString by a toText method in Printer to get access to a Context.
`FunProto#typedArgs` was using the implicit Context from the `FunProto` constructor, and thus errors weren't stored in the proper context. This is fixed by passing an implicit Context argument to `typedArgs` (this was already done for `typedArg`), this means that we no longer need to store a Context in the class, on the other hand there are some new difficulties: - `FunProto#typedArgs` can only be safely called from a point where the implicit Context contains the Context where the `FunProto` instance was created in its owner chain. This is usually the case but required some changes to Implicits (done in the previous commit) - The Context used must be created with Context#thisCallArgContext when typing the arguments of a this(...) constructor call. This requires passing storing this information in FunProto. This wouldn't be necessary if we could simply type the whole this(...) Apply node with Context#thisCallArgContext, unfortunately this does not work since this Context does not have the constructors of the class in scope, and I did not figure out a way to construct a Context that would have them in scope without having the other methods of the class in scope.
smarter
force-pushed
the
fix-spurious-errors
branch
from
66801cc to
04c0f66
Compare
In several situations, instead of the common pattern in the compiler of using the enclosing Context available coming from an implicit method parameter, we used some other Context. Changing this is necessary for scala#2922 to go through. scala#2922 is currently on hold and may never get in but the Context handling simplifications seem worth having anyway: - RunInfo does not actually need to keep a reference to the root Context: It was previously used by `ImplicitRunInfo#implicitScope`, but can be replaced by using the Context from the enclosing scope (which was already used in this method with the name `liftingCtx`). - OfTypeImplicits does not need to keep a reference to the root Context either, we can just replace its lazy vals by defs that cache their results. - ContextualImplicits does need to keep track of the Context from which its implicit references come from, but this Context itself does not have to be available implicitly in the class, instead the Context from the enclosing scope is used. This also requires replacing OfTypeImplicits#toString and ContextualImplicits#toString by a toText method in Printer to get access to a Context.
|
Superceded by #4871 |
No description provided.