-
Notifications
You must be signed in to change notification settings - Fork 204
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
Make resolve follow FileOptions.TopLevelControl #510
Conversation
The new struct FileOptions allows for fine grained control over language feature. However, it didn't respect FileOptions.TopLevelControl flag as the only way to actually allow if/for/while in the top level context was to enable FileOptions.GlobalReassign 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.
Thanks for this contribution!
resolve/resolve.go
Outdated
@@ -505,7 +505,7 @@ func (r *resolver) stmt(stmt syntax.Stmt) { | |||
} | |||
|
|||
case *syntax.IfStmt: | |||
if !r.options.GlobalReassign && r.container().function == nil { | |||
if !r.options.GlobalReassign && !r.options.TopLevelControl && r.container().function == nil { |
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 think these three should just be if !r.options.TopLevelControl ...
, since the GlobalReassign flag was split into two fine-grained options fields.
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.
Done.
@@ -16,14 +16,14 @@ import ( | |||
|
|||
// A test may enable non-standard options by containing (e.g.) "option:recursion". | |||
func getOptions(src string) *syntax.FileOptions { | |||
// TODO(adonovan): use new fine-grained names. |
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.
The intent of this TODO was to get rid of the test option globalreassign
and express it in terms of the fine-grained flags. When the TODO is done, every field in getOptions' FileOptions will be set by exactly one option(src, "...")
call. Would you care to make that change?
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 think I did that. I removed the new tests from resolve.star
as that functionality was already tested in others. I updated the flags in the tests according to this suggestion.
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.
Looks great, thank you!
The new struct
FileOptions
allows for fine grained control over language feature.However, it didn't respect
FileOptions.TopLevelControl
flag, as the only way to actually allow if/for/while in the top level context was to enableFileOptions.GlobalReassign
flag.