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

Prepare lang-item {AST, HIR}::PathInExpressions #3378

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

CohenArthur
Copy link
Member

This is an important base for desugaring expression lang items, e.g. when calling something like IntoIterator::into_iter(i). This changes our AST and HIR visitors to make it possible to use lang items in more places which will be necessary.

gcc/rust/ChangeLog:

	* ast/rust-collect-lang-items.h: Declare visitor.
	* ast/rust-collect-lang-items.cc (CollectLangItems::visit): New.
gcc/rust/ChangeLog:

	* util/rust-hir-map.cc (Mappings::get_lang_item_node): Better formatting when a lang
	item does not exist when it should.
@CohenArthur CohenArthur added this to the Question mark operator milestone Jan 22, 2025
gcc/rust/ast/rust-ast-collector.cc Outdated Show resolved Hide resolved
gcc/rust/ast/rust-ast-visitor.cc Outdated Show resolved Hide resolved
gcc/rust/backend/rust-compile-resolve-path.cc Outdated Show resolved Hide resolved
gcc/rust/backend/rust-compile-resolve-path.cc Outdated Show resolved Hide resolved
gcc/rust/checks/lints/rust-lint-marklive.cc Outdated Show resolved Hide resolved
gcc/rust/hir/tree/rust-hir.cc Outdated Show resolved Hide resolved
gcc/rust/resolve/rust-ast-resolve-path.cc Outdated Show resolved Hide resolved
gcc/rust/resolve/rust-early-name-resolver.cc Outdated Show resolved Hide resolved
Comment on lines +196 to +218
// FIXME: We probably need to check *if* the type needs substitutions
// or not
if (LangItem::IsEnumVariant (expr.get_lang_item ()))
{
std::pair<HIR::Enum *, HIR::EnumItem *> enum_item_lookup
= mappings.lookup_hir_enumitem (*hir_id);
bool enum_item_ok = enum_item_lookup.first != nullptr
&& enum_item_lookup.second != nullptr;
rust_assert (enum_item_ok);

HirId variant_id
= enum_item_lookup.second->get_mappings ().get_hirid ();

HIR::EnumItem *enum_item = enum_item_lookup.second;
resolved_node_id = enum_item->get_mappings ().get_nodeid ();

// insert the id of the variant we are resolved to
context->insert_variant_definition (expr.get_mappings ().get_hirid (),
variant_id);

query_type (variant_id, &infered);
infered = SubstMapper::InferSubst (infered, expr.get_locus ());
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is super dodgy, @philberty please check it good lol

Copy link
Contributor

@liamnaddell liamnaddell Jan 22, 2025

Choose a reason for hiding this comment

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

If I could ask, how could you end up with a lang-item path that DOESN'T need substitution inferencing? I'm curious because I think the only way you could generate an option type that doesn't need inferencing would be to generate a lang-item path like Option::<i32>::Some(3), but that wouldn't be possible since lang-item paths wouldn't have PathExprSegments which can be generic arguments

Copy link
Contributor

Choose a reason for hiding this comment

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

The only other way would be to have a lang-item path to an enumeration variant with no generic parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah maybe something like that, but you're right that I don't think it's the case - there aren't that many enum variants long items, and I think they are all generic. Still, it feels wrong to always do substitution mapping lol so that's why I added the comment. It might also evolve in the future. I'm happy removing the comment though

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's probably better to keep it I would imagine, since if it ever becomes an issue, the comment clarifies that ALWAYS doing substitution inference is not a design choice

Copy link
Member

Choose a reason for hiding this comment

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

there might be a change needed for the generics but would need to try it out a bit myself to try our some tests on it

gcc/rust/util/rust-lang-item.cc Outdated Show resolved Hide resolved
@CohenArthur CohenArthur force-pushed the prepare-lang-item-pathinexpressions branch from aa6bcef to 19b0e5f Compare January 22, 2025 16:59
gcc/rust/ChangeLog:

	* util/rust-lang-item.cc (LangItem::IsEnumVariant): New function.
	* util/rust-lang-item.h: Declare it.
gcc/rust/ChangeLog:

	* ast/rust-ast-collector.cc (TokenCollector::visit): Adapt visitor to lang item
	PathInExpressions.
	* ast/rust-ast-visitor.cc (DefaultASTVisitor::visit): Likewise.
	* expand/rust-cfg-strip.cc (CfgStrip::visit): Likewise.
	* expand/rust-expand-visitor.cc (ExpandVisitor::visit): Likewise.
	* hir/rust-ast-lower.cc (ASTLoweringExprWithBlock::visit): Likewise.
	(ASTLowerPathInExpression::visit): Likewise.
	* resolve/rust-ast-resolve-path.cc (ResolvePath::resolve_path): Likewise.
	* resolve/rust-early-name-resolver.cc (EarlyNameResolver::visit): Likewise.
gcc/rust/ChangeLog:

	* hir/tree/rust-hir-path.h: Adapt PathPattern to accept lang-item paths.
	* hir/tree/rust-hir-path.cc: Assert we are dealing with a segmented path, create lang-item
	constructors.
	* hir/tree/rust-hir.cc (PathPattern::convert_to_simple_path): Likewise.
gcc/rust/ChangeLog:

	* backend/rust-compile-resolve-path.cc (ResolvePathRef::visit): Adapt visitor to lang item
	HIR::PathInExpressions.
	* typecheck/rust-hir-type-check-path.cc (TypeCheckExpr::visit): Likewise.
gcc/rust/ChangeLog:

	* checks/lints/rust-lint-marklive.cc (MarkLive::visit): Adapt to lang items.
@CohenArthur CohenArthur force-pushed the prepare-lang-item-pathinexpressions branch from 19b0e5f to 3e31b59 Compare January 22, 2025 17:03
Copy link
Contributor

@liamnaddell liamnaddell left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 40 to 41
final_segment
= HIR::PathIdentSegment (LangItem::ToString (expr.get_lang_item ()));
Copy link
Contributor

@liamnaddell liamnaddell Jan 22, 2025

Choose a reason for hiding this comment

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

Could this end up getting confused if there are multiple Some declarations floating around?

I.e.

fn Some () {
 let _ = option_env!("PWD"); // Yields lang-item path to Some("/tmp")
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe - I'm adding a different implementation right now which is more precise and makes use of the lang item's known NodeId

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the version you have up now works, at least from tracing the code by eye.

gcc/rust/ChangeLog:

	* ast/rust-path.h: New function.
@CohenArthur CohenArthur force-pushed the prepare-lang-item-pathinexpressions branch from 3e31b59 to 70384f2 Compare January 23, 2025 12:15
Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

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

LGTM lets get this merged soon

gcc/rust/ChangeLog:

	* backend/rust-compile-resolve-path.cc (ResolvePathRef::visit): Call into
	resolve_path_like instead.
	(ResolvePathRef::resolve_path_like): New.
	(ResolvePathRef::resolve): Call into resolve_with_node_id.
	* backend/rust-compile-resolve-path.h: Declare new functions and document them.
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.

3 participants