Skip to content
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

Fix implicit parameter list capture #52

Merged
merged 1 commit into from
Jun 22, 2021

Conversation

keynmol
Copy link
Collaborator

@keynmol keynmol commented Jun 18, 2021

This particular case seems to have slipped through in #51

Wasn't working before Oli's PR as well.


Problem is breaking apart multiple argument lists - adding recordValue in the wrong place (between an implicit and non-implicit parameter list), seems to break things.

The way to fix it is to avoid putting recorders on the implicit parameters (not tearing them away from a method call).

This, obviously, is the stuff of nightmares in Scala 3, where you can do anything:

scala> def hello(x: String)(using Int, List[String])(t: Double)(using what: Set[Int]) = ???
def hello
  (x: String)
    (using x$2: Int, x$3: List[String])
      (t: Double)(using what: Set[Int]): Nothing

@keynmol keynmol changed the title Regression with implicit parameters Another problem with implicit parameters Jun 18, 2021
@keynmol keynmol changed the title Another problem with implicit parameters Fix implicit parameter list capture Jun 18, 2021
case Apply(x, ys) => Apply(recordAllValues(x), ys.map(recordAllValues))
case Apply(x, ys) =>
val allParametersAreImplicit =
ys.map(x => Option(x.symbol).fold(false)(_.isImplicit)).forall(_ == true)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.symbol can be null 🎉 :D

@eed3si9n
Copy link
Owner

Nice. Thanks for the fix!

@keynmol
Copy link
Collaborator Author

keynmol commented Jun 18, 2021

The journey is not over, apparently :D I spotted this in my own project, and even publishing with this fix doesn't seem to help it.

I will keep reproducing and fixing I guess :-/

@keynmol
Copy link
Collaborator Author

keynmol commented Jun 19, 2021

Gonna keep my notes

I added a Scala3-only test, using givens instead of implicits. And that's what broke. Because in the AST, if you use givens:

------------------------------                                        
expression: z.hello[scala.Int](50)(given_Test_Int)                                
parameters: List(Ident(given_Test_Int))
flags: List(Flags.Final | Flags.Lazy | Flags.Module | Flags.StableRealizable)
apply on: z.hello[scala.Int](50)                                                

But if you use implicits:

------------------------------ 
z.hello[scala.Int](50)(testInt)
List(Ident(testInt))           
List(Flags.Implicit)           
z.hello[scala.Int](50)         

And the problem is that using givens this is not passing the flags check :(

@keynmol
Copy link
Collaborator Author

keynmol commented Jun 19, 2021

@eed3si9n @Baccata I think this is in a better state for review.

The reason original fix was insufficient is that Flags.Given is not showing up on the parameters: https://github.com/lampepfl/dotty/blob/master/library/src/scala/quoted/Quotes.scala#L3968

For the simple reason that it's not mentioned here :) https://github.com/lampepfl/dotty/blob/master/compiler/src/scala/quoted/runtime/impl/printers/Extractors.scala#L37

@keynmol keynmol requested review from eed3si9n and Baccata and removed request for eed3si9n June 22, 2021 19:47
Copy link
Owner

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squash the commits please, and I think it's good to go.

@keynmol keynmol force-pushed the reprodce-regression branch from dedd3de to 8fc7d18 Compare June 22, 2021 20:15
@keynmol keynmol merged commit 1a4c23c into eed3si9n:develop Jun 22, 2021
@keynmol keynmol deleted the reprodce-regression branch June 22, 2021 20:23
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