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.

Sign up for GitHub

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

Jump to bottom

Fix pretty printer to handle using and erased modifier #17952

Merged
merged 1 commit into from
Jul 28, 2023
Merged

Fix pretty printer to handle using and erased modifier #17952

merged 1 commit into from
Jul 28, 2023

Conversation

fmonniot
Copy link
Contributor

@fmonniot fmonniot commented Jun 10, 2023 •
edited
Loading

The existing pretty printer doesn't handle using/erased keyword, making it a bit misleading when investigating code generated by macros.

This PR add support for those two modifiers

smarter
smarter approved these changes Jun 10, 2023
View reviewed changes
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

LGTM. @nicolasstucki: is this a good way to test the output of the tasty printer?

nicolasstucki
nicolasstucki suggested changes Jun 12, 2023
View reviewed changes
compiler/src/scala/quoted/runtime/impl/printers/SourceCode.scala Outdated
inParens {
if (argFlags.is(Flags.Implicit) && !argFlags.is(Flags.Given)) this += "implicit "
if (argFlags.is(Flags.Given)) this += "using "
if (argFlags.is(Flags.Erased)) this += "erased "
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is not correct, erased can appear on each individual parameter.

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'm hoping 7f9208b fixed this. Do let me know if you think it's not in the right place.

tests/run-macros/term-show/Test_2.scala Show resolved Hide resolved
tests/run-macros/term-show/Macro_1.scala Outdated Show resolved Hide resolved
compiler/src/scala/quoted/runtime/impl/printers/SourceCode.scala
inParens {
if (argFlags.is(Flags.Implicit) && !argFlags.is(Flags.Given)) this += "implicit "
if (argFlags.is(Flags.Given)) this += "using "
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not print this if these are the parameters of a lambda. See tests/run-staging/quote-nested-1.check.

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'm not entirely sure to understand this one. Are you saying that if the arg flag includes given but the outer type is a lambda then the keyword using is invalid because it should be a contextual function type ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, you meant that the test case failed. I should have checked CI before asking the question above. Will take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0910940 is an attempt to ignore lambdas, although it feels very targeted to me. Let me know what you think of it.

Copy link
Contributor

@nicolasstucki nicolasstucki Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe it would be better to split into printMethdArgsDefs and printLambdaArgsDefs as those are different in the grammar.

Copy link
Contributor Author

@fmonniot fmonniot Jul 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Addressed in 34c7058. Let me know if that seems more readable to you or not.

Copy link
Member

Choose a reason for hiding this comment

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

Addressed in 34c7058. Let me know if that seems more readable to you or not.

That seems fine to me, but I would deduplicate the two printSeparated local defs into one private def reused by both methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done via 25f78ea.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can simplify this to

- if (argFlags.is(Flags.Implicit) && !argFlags.is(Flags.Given)) this += "implicit "
- if (argFlags.is(Flags.Given)) this += "using "
+ if argFlags.is(Flags.Given) then this += "using "
+ else if argFlags.is(Flags.Implicit) then this += "implicit "

tests/run-macros/term-show/Macro_1.scala Outdated Show resolved Hide resolved
@smarter
Copy link
Member

smarter commented Jun 23, 2023

There's a conflict with the main branch because #16968 also added a test named term-show, you can fix that by either renaming your test or combining the two tests together.

@fmonniot fmonniot force-pushed the quotes-using-printer branch from 0910940 to 34c7058 Compare July 22, 2023 00:39
@fmonniot
Copy link
Contributor Author

Sorry for the delay. I have rebased against the latest main branch and −hopefully− addressed the remaining review comment.

smarter
smarter approved these changes Jul 25, 2023
View reviewed changes
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

compiler/src/scala/quoted/runtime/impl/printers/SourceCode.scala
inParens {
if (argFlags.is(Flags.Implicit) && !argFlags.is(Flags.Given)) this += "implicit "
if (argFlags.is(Flags.Given)) this += "using "
Copy link
Member

Choose a reason for hiding this comment

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

Addressed in 34c7058. Let me know if that seems more readable to you or not.

That seems fine to me, but I would deduplicate the two printSeparated local defs into one private def reused by both methods.

nicolasstucki
nicolasstucki reviewed Jul 26, 2023
View reviewed changes
tests/run-macros/term-show/Test_2.scala Outdated
@@ -25,5 +37,17 @@ object Test {
| }
| ()
|}""".stripMargin)

assert(showTree("A")
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to move this check into a checkfiles. They are simpler to update/debug if the format changes.

Suggested change
assert(showTree("A")
println(showTree("A"))

Add tests/run-macros/term-show.check then run scala3-bootstrapped/testCompilatation tests/run-macros/term-show. Follow the instructions of the error message; the mv command printed will update the checkfile.

Copy link
Contributor Author

@fmonniot fmonniot Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's what we had before the merge with main, but #16968 used a different style and I followed what was already in main.

@nicolasstucki Should I refactor both asserts to use checkfiles for consistency ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it would be good to refactor both. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 6afd86d.

@nicolasstucki
Copy link
Contributor

@fmonniot could you squash the commits?

@fmonniot fmonniot force-pushed the quotes-using-printer branch from 6afd86d to 056fbc5 Compare July 27, 2023 22:39
@fmonniot
Copy link
Contributor Author

@nicolasstucki done

@nicolasstucki nicolasstucki merged commit 5f224a4 into scala:main Jul 28, 2023
@fmonniot fmonniot deleted the quotes-using-printer branch July 28, 2023 13:05
@Kordyjan Kordyjan mentioned this pull request Dec 1, 2023
Merged
Kordyjan added a commit that referenced this pull request Dec 8, 2023
@Kordyjan
Backport "Fix pretty printer to handle using and erased modifier" to …
2f62a6f 
…LTS (#19137)

Backports #17952 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
@Kordyjan Kordyjan added this to the 3.3.2 milestone Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolasstucki nicolasstucki nicolasstucki approved these changes

@smarter smarter smarter approved these changes

Assignees

@nicolasstucki nicolasstucki

Labels
None yet
Projects
None yet
Milestone
3.3.2
Development

Successfully merging this pull request may close these issues.
4 participants
@fmonniot @smarter @nicolasstucki @Kordyjan