-
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
Stabilise SIP-47 (Adding Clause Interleaving to method definitions) #20861
Conversation
6072739
to
7ff2398
Compare
This PR is based on top of #20895 |
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.
Otherwise LGTM
@@ -3878,13 +3878,11 @@ object Parsers { | |||
val ident = termIdent() | |||
var name = ident.name.asTermName | |||
val paramss = | |||
if in.featureEnabled(Feature.clauseInterleaving) then | |||
// If you are making interleaving stable manually, please refer to the PR introducing it instead, section "How to make non-experimental" | |||
if Feature.clauseInterleavingEnabled(using in.languageImportContext) then |
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.
This looks out of line with other code that's version dependent, Why the using in.languageImportContext
?
That's done nowhere else in the parser. Why not just the original test?
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.
There's a doc comment further up that needs to be updated.
* if clauseInterleaving is enabled:
* DefSig ::= id [DefParamClauses] [DefImplicitClause]
*
As far as Parser is concerned, clauseInterleaving is now enabled, so the grammar should reflect this.
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.
This looks out of line with other code that's version dependent, Why the using in.languageImportContext?
That's done nowhere else in the parser. Why not just the original test?
The issue there is that we also need to check if -source
is configured and if it is less than 3.6
, we should require the language import. While in.featureEnabled(Feature.clauseInterleaving)
only checks that there is an import statement, it doesn't check the source version. Feature.clauseInterleavingEnabled
does both of them but it seems that the context doesn't contain that information and the in.languageImportContext
does (which is exactly what in.featureEnabled
does)
I will wait for #20895 to be merged before merging this PR |
@@ -121,7 +121,8 @@ object Feature: | |||
|
|||
def namedTypeArgsEnabled(using Context) = enabled(namedTypeArguments) | |||
|
|||
def clauseInterleavingEnabled(using Context) = enabled(clauseInterleaving) | |||
def clauseInterleavingEnabled(using Context) = | |||
sourceVersion.isAtLeast(`3.6`) || enabled(clauseInterleaving) |
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.
this shouldn't be mixing source version with language import checks, they should be kept separate,
e.g. in the parser do
if sourceVersion.isAtLeast(`3.6`) || in.featureEnabled(Feature.clauseInterleaving) then
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.
not sure to see why we should do that ?
edit: fewerBraces
was stabilised in 3.3
and we do similarly:
def fewerBracesEnabled(using Context) = |
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.
ok, I guess its fine then, curious that in parser they just have Feature.fewerBracesEnabled
without explicit using
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.
Yea, I was currious too. I guess it might be a no-op as it is, therefore a bug ?
This has been decided to be included in the 3.6.0 release. |
should #21195 be a blocker for this? |
I don't think so. It only affects new code combined with Java code. It cannot break anyone immediately. We should fix it, though, for sure, but not a blocker IMO. |
okay, it was open as a discussion but I've made it into an issue, #21346 |
Closes #20769
Initial implementation in #14019