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

Partially fix bug in type resolution of paths #3277

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

powerboat9
Copy link
Contributor

@powerboat9 powerboat9 commented Dec 1, 2024

When name resolution 2.0 is enabled these sections of the typechecker should be performing resolution lookups using segment node ids, rather than path node ids, as is done when name resolution 1.0 is enabled. Since name resolution 2.0 doesn't currently insert resolutions for individual segments, lookups based on path node id are left as a fallback.

Comment on lines +221 to +231
nr_ctx.lookup (ast_node_id).map ([&ref_node_id] (NodeId resolved) {
ref_node_id = resolved;
});

// HACK: on failure, redo lookup using path node id
// This is incorrect, but will help some tests pass for now
// TODO: remove this
if (ref_node_id == UNKNOWN_NODEID)
nr_ctx.lookup (expr.get_mappings ().get_nodeid ())
.map (
[&ref_node_id] (NodeId resolved) { ref_node_id = resolved; });
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 I would rather spend some time fixing the root cause for this. This seems a bit dodgy. what do you think is missing from nr2.0 in order to enable this? simply resolving each segment when we resolve a path?

Copy link
Member

Choose a reason for hiding this comment

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

do your lookup on the ast_node_it which is the segment not the path name resolution cant resolve a full path on its own.

.map ([&ref_node_id, &path] (NodeId resolved) {
ref_node_id = resolved;
});
nr_ctx.lookup (ast_node_id).map ([&ref_node_id] (NodeId resolved) {
Copy link
Member

Choose a reason for hiding this comment

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

you need to be looking at the segment node_id.

The name resolution for Paths is a really huge pain in the ass. this is resolving the ROOT of a path so for example you could have:

mod_a::Type::impl_item

the root path should just resolve this:

mod_a::Foo then stop then fall back to the resolve segments to then find the impl_item

So i think you should just be using the ast_node_id like i do in the old name resolver here.

gcc/rust/ChangeLog:

	* typecheck/rust-hir-type-check-path.cc
	(TypeCheckExpr::resolve_root_path): Use segment node ids instead
	of the path node id to look up segment resolutions in the 2.0
	resolver, like is done with the 1.0 resolver, when possible.
	* typecheck/rust-hir-type-check-type.cc
	(TypeCheckType::resolve_root_path): Likewise, also remove
	unnecessary capture of path in lambda.

Signed-off-by: Owen Avery <[email protected]>
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