Skip to content

Do the implicit search shadowing check in the correct context #1142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 7, 2016

Conversation

smarter
Copy link
Member

@smarter smarter commented Mar 3, 2016

This commit fixes a very sneaky bug, the following code:

lazy val shadowing =
  typed(untpd.Ident(ref.name) withPos pos.toSynthetic, funProto)
       (nestedContext.addMode(Mode.ImplicitShadowing).setExploreTyperState)

is parsed by scalac as:

lazy val shadowing =
  typed(untpd.Ident(ref.name) withPos pos.toSynthetic, funProto);
(nestedContext.addMode(Mode.ImplicitShadowing).setExploreTyperState);

So we don't actually use the nested context in typed, instead we end
up implicitly using ctx!

Review by @odersky

This commit fixes a very sneaky bug, the following code:
```
lazy val shadowing =
  typed(untpd.Ident(ref.name) withPos pos.toSynthetic, funProto)
       (nestedContext.addMode(Mode.ImplicitShadowing).setExploreTyperState)
```
is parsed by scalac as:
```
lazy val shadowing =
  typed(untpd.Ident(ref.name) withPos pos.toSynthetic, funProto);
(nestedContext.addMode(Mode.ImplicitShadowing).setExploreTyperState);
```
So we don't actually use the nested context in `typed`, instead we end
up implicitly using `ctx`!
@smarter
Copy link
Member Author

smarter commented Mar 3, 2016

This sort of bug is the reason why GCC 6 added a warning -Wmisleading-indentation, I wonder if we could do the same in scala.

@smarter
Copy link
Member Author

smarter commented Mar 3, 2016

The tests now fail with assertion failed: inconsistent: some typevars were added to uncommittable constraint.

@odersky : What's the harm of adding typevars to an uncommittable constraint set? Could we bypass this check?

We triggered this assert after the fix in the previous commit.
@smarter
Copy link
Member Author

smarter commented Mar 4, 2016

Changing the assertion check was enough to make all tests pass, so I think this is good to go now.

@odersky
Copy link
Contributor

odersky commented Mar 7, 2016

LGTM

odersky added a commit that referenced this pull request Mar 7, 2016
Do the implicit search shadowing check in the correct context
@odersky odersky merged commit 742ae75 into scala:master Mar 7, 2016
@allanrenucci allanrenucci deleted the fix/implicit-ctx branch December 14, 2017 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants