Skip to content
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

Closed
wants to merge 129 commits into from
Closed

Structs in PIL #1516

wants to merge 129 commits into from

Conversation

gzanitti
Copy link
Collaborator

@gzanitti gzanitti commented Jul 1, 2024

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 struct X {x: int, y: int} is created using X with {x: 1, y:0}.

@gzanitti gzanitti marked this pull request as ready for review October 11, 2024 00:44
@@ -50,10 +50,28 @@ pub struct PILGraph {

#[derive(Clone)]
pub enum TypeOrExpression {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeDeclarationOrExpression?

Copy link
Collaborator Author

@gzanitti gzanitti Oct 14, 2024

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

ast/src/object/mod.rs Outdated Show resolved Hide resolved
},
) => {
let mut exp_fields = Vec::new();
for NamedExpression { name, body } in fields.iter() {
Copy link
Member

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)

Copy link
Collaborator Author

@gzanitti gzanitti Oct 14, 2024

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?

Copy link
Member

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
Copy link
Member

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.

Copy link
Collaborator Author

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).

Copy link
Member

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.

@gzanitti
Copy link
Collaborator Author

This has been simplified into the following PRs:

  1. Definitions and statement processor: Structs: Definitions & Stmt processor #1906
  2. Field checking before type checking: Structs: Fields check #1907
  3. Type inference: Structs: Type inference  #1910
  4. Condenser & Evaluator: Structs: Condenser & Evaluator #1911

@gzanitti gzanitti closed this Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants