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

Borrowck ast lifetimes #2771

Merged
merged 5 commits into from
Dec 27, 2023
Merged

Conversation

jdupak
Copy link
Contributor

@jdupak jdupak commented Dec 4, 2023

Depends on #2770

Fixes #2756

@P-E-P P-E-P added this to the Borrow Checking 1 milestone Dec 6, 2023
@jdupak jdupak force-pushed the borrowck-ast-lifetimes branch 2 times, most recently from 5377af2 to b82aa74 Compare December 11, 2023 22:35
There was a mismatch whether lifetime 'static is parsed as "static"
or "'static".

gcc/rust/ChangeLog:

	* parse/rust-parse-impl.h (Parser::lifetime_from_token): Fix matched pattern.

Signed-off-by: Jakub Dupak <[email protected]>
gcc/rust/ChangeLog:

	* ast/rust-ast.h: Elided lifetime static constructor
	* ast/rust-type.h: Default lifetime to elided.
	* parse/rust-parse-impl.h (Parser::parse_lifetime_param): Use elided lifetime.
	(Parser::parse_lifetime): Use elided lifetime/
	(Parser::lifetime_from_token): Use elided lifetime.
	(Parser::parse_self_param): Use elided lifetime.
	(Parser::parse_reference_type_inner): Use elided lifetime.

Signed-off-by: Jakub Dupak <[email protected]>
gcc/rust/ChangeLog:

	* parse/rust-parse-impl.h (Parser::parse_generic_param): Lifetime elision control.
	(Parser::parse_lifetime_where_clause_item): Lifetime elision control.
	(Parser::parse_type_param_bound): Lifetime elision control.
	(Parser::parse_lifetime_bounds): Lifetime elision control.
	(Parser::parse_lifetime): Lifetime elision control.
	(Parser::parse_path_generic_args): Lifetime elision control.
	(Parser::parse_self_param): Lifetime elision control.
	(Parser::parse_break_expr): Lifetime elision control.
	(Parser::parse_continue_expr): Lifetime elision control.
	(Parser::parse_reference_type_inner): Lifetime elision control.
	* parse/rust-parse.h: Lifetime elision control.

Signed-off-by: Jakub Dupak <[email protected]>
(probably incomplete propagation)

gcc/rust/ChangeLog:

	* hir/rust-ast-lower-base.cc (ASTLoweringBase::lower_lifetime): Propagate static
	requirement.
	* hir/rust-ast-lower-base.h: Propagate static requirement.
	* hir/rust-ast-lower-implitem.h: Propagate static requirement.
	* hir/rust-ast-lower-item.cc (ASTLoweringItem::visit): Propagate static requirement.
	* hir/rust-ast-lower-type.cc (ASTLoweringType::translate): Propagate static requirement.
	(ASTLoweringType::visit): Propagate static requirement.
	* hir/rust-ast-lower-type.h: Propagate static requirement.

Signed-off-by: Jakub Dupak <[email protected]>
gcc/rust/ChangeLog:

	* hir/rust-ast-lower-type.cc (ASTLoweringType::visit): For lifetimes.

Signed-off-by: Jakub Dupak <[email protected]>
@jdupak jdupak force-pushed the borrowck-ast-lifetimes branch from b82aa74 to 71f7686 Compare December 19, 2023 21:55
@jdupak jdupak marked this pull request as ready for review December 19, 2023 21:55
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

looks good :) thanks for all the work ❤️

Comment on lines +537 to +541
if (lifetime_type == AST::Lifetime::WILDCARD && default_to_static_lifetime)
{
// If compiling in a static context.
lifetime_type = AST::Lifetime::STATIC;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (lifetime_type == AST::Lifetime::WILDCARD && default_to_static_lifetime)
{
// If compiling in a static context.
lifetime_type = AST::Lifetime::STATIC;
}
// If compiling in a static context.
if (lifetime_type == AST::Lifetime::WILDCARD && default_to_static_lifetime)
lifetime_type = AST::Lifetime::STATIC;

GNU style nit, sorry

{}

/** Used when compiling const and static items. */
bool default_to_static_lifetime;
Copy link
Member

Choose a reason for hiding this comment

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

I don't get how this flag is used or set. If we are lowering an AST::ConstantItem or AST::StaticItem, we know we are doing so and can pass an explicit true flag to ASTLoweringType (or even better, a LifetimeDefault::Static enum variant which is clearer). and we could have a default value to the bool argument to the ASTLoweringType argument?

does that make sense? or is it important that we keep it this way for future pull-requests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This flag is used to get the state across the visitor API and propagate further ASTLoweringType::translate calls since the visitor API does not allow more parameters. I think it makes sense. Does it make sense now?

Copy link
Member

Choose a reason for hiding this comment

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

ah, yeah I see what you mean. so we really can't do without a default parameter then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the member variable or the parameter? We need the member variable, the parameter is useful because we don't need to change it everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

I meant the parameter - but I'm fine with keeping it this way, your explanation makes sense. I'll merge this now

@CohenArthur CohenArthur added this pull request to the merge queue Dec 27, 2023
Merged via the queue into Rust-GCC:master with commit b9fccab Dec 27, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

Special lifetime indentifiers name not matched
3 participants