-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add regression test for issue 18726 #20318
Conversation
76a68cb
to
5bb98db
Compare
5bb98db
to
f5dc97f
Compare
check( | ||
""" | ||
|object A { | ||
| "".repea@@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this actually working? I think I actually had to turn it off and -release options is removed:
scala3/presentation-compiler/src/main/dotty/tools/pc/ScalaPresentationCompiler.scala
Line 61 in 5e83606
private val forbiddenDoubleOptions = Set("-release") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I don't understand the question. Are you asking about how the test is implemented or how are we handling the release flag ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are the tests working? And maybe we need to stop removing the flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I guess it is because I passed it as -release:8
and it is a single argument on the list, thus it is not removed ? We should remove this line tho beacuse -release 8
is a valid scala flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you remember why we added this check in the first place ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might have been interfering with some other options and it seemed not important for frontend phases scalameta/metals#3812
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested it and seems to work ok with that removed.
Co-authored-by: Tomasz Godzik <tgodzik@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Closes #18726
[test_windows_full]