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

NR2: Use parent scope instead of enum scope to resolve enum content #3430

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

Conversation

P-E-P
Copy link
Member

@P-E-P P-E-P commented Feb 12, 2025

struct E2 /* (1) */;

enum Test {
    E1,
    E2 /* (2) */ (E2 /* (3) */),
}

E2 (3) is resolved as E2 (2) instead of E2(1) because enum's scope has been pushed. In Late::visit(TypePath &type) we should map the usage to E2(1) definition.

For now I'm trying to find an elegant solution (resolve the TypePath from it's scope's parent) but this should only work for Enum (Reuse ContextualVisitor ?).

I'm not a fan of big forever stack modification, and obviously modifying it for that kind of edge case feels off.

  • We could introduce a new resolve_from_parent instead of that extra boolean
  • If we keep that extra argument I should probably change it to an explicit enum
  • Should we introduce a function to resolve from anywhere instead of the node's parent ? Would it be used anywhere ?
  • Should we look for parent module instead of parent scope ?

Should fix nr2 test exhaustiveness2.rs

gcc/rust/ChangeLog:

	* resolve/rust-forever-stack.h:
	* resolve/rust-forever-stack.hxx:
	* resolve/rust-late-name-resolver-2.0.cc (Late::visit):
	* resolve/rust-name-resolution-context.h:

Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
@P-E-P P-E-P marked this pull request as draft February 12, 2025 16:31
@P-E-P P-E-P requested a review from powerboat9 February 12, 2025 17:02
@P-E-P P-E-P self-assigned this Feb 12, 2025
@P-E-P
Copy link
Member Author

P-E-P commented Feb 12, 2025

@powerboat9 anything to say about this ? I've taken a look at some of your PRs and I don't think you've introduced any "resolved from given scope" function right ?

@powerboat9
Copy link
Collaborator

It doesn't look like it'll be quite that simple. I think skipping enum ribs when attempting to resolve the first segment of a path would do it.

@P-E-P
Copy link
Member Author

P-E-P commented Feb 12, 2025

Exactly what I was looking for, a counter example.

@CohenArthur
Copy link
Member

I don't think there's any reason for E2 to resolve to the enum variant in the first place, it should only be possible if the variant has been imported by a use statement unless I'm forgetting something. But I don't think you can ever reach E2 without typing Test::E2 so we are doing something wrong indeed. So I wonder if we couldn't fix this by not adding new definitions for enum variants or something of the sort, but it might just be easier to skip enum ribs like powerboat mentioned

@powerboat9
Copy link
Collaborator

Can I try a fix?

@P-E-P
Copy link
Member Author

P-E-P commented Feb 25, 2025

Can I try a fix?

I've been on vacation for the past week hence the delayed answer/no activity on this PR, actually I'd like to tackle it myself.

@powerboat9
Copy link
Collaborator

Can I try a fix?

I've been on vacation for the past week hence the delayed answer/no activity on this PR, actually I'd like to tackle it myself.

Oh, yeah, that's fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants