-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Re-design Env of the global object init checker #24107
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
Re-design Env of the global object init checker #24107
Conversation
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 this is OK, but is there a rationale for the inconsistency that we use the rhs as the tree to identify a lazy val but the overall DefDef as the tree to identify a method (as opposed to the rhs of the method)?
If there is a rationale, it would be good to record it in a comment.
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.
Documented the rationale. We must use the overall DefDef as the tree to identify a method (as opposed to the rhs of the method) because resolveEnvByMethod looks for the symbol of the DefDef to see it if matches the target method
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.
Why is the outerEnv NoEnv? Can't the rhs of a lazy val refer to things from the enclosing scope of its definition site?
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.
For lazy fields, their outer scope is a template and they can refer to other fields. However, currently we do not create environments that encapsulate states of instances, so the outerEnv is NoEnv for lazy fields. The values of other fields can be found by searching through thisV instead, and thisV is an InstanceRef with its fields and outer environments.
Another possible design is to store all outer environments as a chain in EnvMap and bypasses the InstanceRef, or even creates environments for instantiation. Do you prefer any of these designs? Maybe we can modify it in another PR
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, that makes sense. Thanks for the explanation. We can keep the current design.
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.
Same as above: why NoEnv?
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 document for the class EnvRef is outdated.
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.
Will there a problem with equality? The tree will have structural equality, while previously the symbol has reference equality.
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 document for the class
EnvRefis outdated.
Updated the document
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.
Will there a problem with equality? The tree will have structural equality, while previously the symbol has reference equality.
Defined the equals method, which checks reference equality of the trees
54205cf to
480a6d5
Compare
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
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 now.
480a6d5 to
98d2ed5
Compare
When the global object init checker models lazy fields and by-name parameters, initially it creates an environment for each symbol of the lazy field/by-name parameter. For by-name parameters, the environment includes all the possible environments that pass to the parameter as outer environments. This PR modifies the design of environments to take the tree to evaluate as the argument. Therefore, the same of argument tree passed to a by-name parameter will share the same environment, and there will be multiple environments for the same by-name parameter. This facilitates the process of finding local variables in the argument tree. Now the process of evaluating by-name args in
multiple-by-name.scalawill behave similarly to evaluating closure arguments inmultiple-func-arg.scala