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

Explicitly upcast intermediate result to aid program elaboration #230

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

julienrf
Copy link
Contributor

@julienrf julienrf commented Jul 5, 2023

See scala/scala3#18130 for context.

We add an explicit type ascription to help the compiler infer contextual arguments. This make the code future-proof against the next versions of the compiler. I have successfully compiled the project locally with Scala 3.3.2-RC1-bin-20230703-0a21ecf-NIGHTLY.

@@ -89,7 +89,7 @@ class ParserJVM private[parser] (pattern: String) extends Parser(pattern) {
* `"[abc]"`
*/
override def charClass[A: P]: P[CharacterClass] =
Indexed("[" ~ "^".!.? ~ (charClassIntersection.rep(exactly = 1) | classItem.rep(minCharClassItem)) ~ "]")
Indexed("[" ~ "^".!.? ~ ((charClassIntersection.rep(exactly = 1): P[Seq[RegexTree]]) | classItem.rep(minCharClassItem)) ~ "]")
Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask why this particular rep needs an upcast, but not others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, thank you for having a look at the PR!

In scala/scala3#17924, I made a change in the compiler that affects program elaboration (ie, implicits resolution). The consequence is that the compiler may give up earlier than what it used to do in the past. Here, the type ascription helps the compiler figure out the least upper bound of both sides of the | operator, which in turn simplifies the resolution of implicit parameter for the second rep call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation! This rep comes from fastparse so I would imagine there’re more projects that depend on fastparse that are affected by this problem. Should this issue be elevated to fastparse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what would be a good fix in fastparse, though…

Copy link
Member

@hugo-vrijswijk hugo-vrijswijk left a comment

Choose a reason for hiding this comment

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

Thanks! I'm excited the community build picked up something in this humble project 🙏. I don't fully understand the bug and why the explicit type is needed, but I am fine with merging this

@hugo-vrijswijk
Copy link
Member

hugo-vrijswijk commented Jul 7, 2023

@julienrf Could you run scalafmt or give me push access to this PR so I can run it and make formatting happy?

See scala/scala3#18130 for context.

We add an explicit type ascription to help the compiler infer contextual arguments.
@julienrf
Copy link
Contributor Author

Sorry for the delay. This is done!

@hugo-vrijswijk
Copy link
Member

Arguably the formatting is less legible, but I dare not go against scalafmt. Thanks for the contribution!

@hugo-vrijswijk hugo-vrijswijk enabled auto-merge (squash) July 10, 2023 07:15
@hugo-vrijswijk hugo-vrijswijk merged commit 3204411 into stryker-mutator:main Jul 10, 2023
8 checks passed
@julienrf julienrf deleted the help-elaboration branch July 10, 2023 07:18
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.

None yet

3 participants