-
Notifications
You must be signed in to change notification settings - Fork 82
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
Structs in PIL #1516
Structs in PIL #1516
Conversation
ast/src/object/mod.rs
Outdated
@@ -50,10 +50,28 @@ pub struct PILGraph { | |||
|
|||
#[derive(Clone)] | |||
pub enum TypeOrExpression { |
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.
TypeDeclarationOrExpression?
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.
In fact, this enum doesn't seem to be used anymore. It seems to have been abandoned after the recent refactor.
EDIT: I opened another PR to remove it: #1904
pil-analyzer/src/evaluator.rs
Outdated
}, | ||
) => { | ||
let mut exp_fields = Vec::new(); | ||
for NamedExpression { name, body } in fields.iter() { |
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.
Consider changing body
to value
inside NamedExpression
(if it makes sense in all cases)
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 now it's used only in Traits and Structs. Since they are NamedExpressions it seems to me that the most accurate/general would be expr
.
What do you think?
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.
Yes, makes sense for NamedExpression
to use name
and expr
:)
name: self.driver.resolve_type_ref(&reference.path), | ||
type_args, | ||
}), | ||
fields: 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.
I think we should check here that all the fields exist and that they are also all mentioned.
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 biggest problem with this is that at this point we don't have the struct declaration. So this check is done during type checking (which was also improved).
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 AnalysisDriver knows about all the symbols, so we could do it at this point. Not saying we should if it's too difficult.
This has been simplified into the following PRs:
|
This PR implements structs based on the issue #1016.
To avoid ambiguity with the grammar, for creating an instance of a struct and for accessing its fields, a temporary variation of the grammar is used which should be discussed.Currently, an instance of structX {x: int, y: int}
is created usingX with {x: 1, y:0}
.