Skip to content

fix: “Generate constant” ignores the path prefix of the identifier #12286

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

Merged
merged 1 commit into from
Jul 18, 2022

Conversation

harpsword
Copy link
Contributor

fix #12022

add abilities to generate constant with prefix path, even these mods in path are not exist.

some examples

multi.path.mov
even.in.different.file.mov
simple.version.mov
generate.constant.which.prefix.not.exists.mov

@harpsword harpsword force-pushed the fix_generate_constant branch from 6376671 to 85ffba3 Compare May 17, 2022 11:59
@Veykril Veykril self-assigned this Jun 21, 2022
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

Sorry this took me so long to get back to, I have lost sight of this PR because it does some things we probably want proper support for (like creating inline modules for a path) and I was thinking about how to do that properly.

With that said, I am still not really sure how to do this nicely so I think its better to merge this now instead of keeping it blocked. So if you could fix the one unwrap usage, I'll go ahead and merge this.

Comment on lines +53 to +75
let name_refs = path.segments().map(|s| s.name_ref());
let mut outer_exists = false;
let mut not_exist_name_ref = Vec::new();
let mut current_module = constant_module;
for name_ref in name_refs {
let name_ref_value = name_ref?;
let name_ref_class = NameRefClass::classify(&ctx.sema, &name_ref_value);
match name_ref_class {
Some(NameRefClass::Definition(Definition::Module(m))) => {
if !m.visibility(ctx.sema.db).is_visible_from(ctx.sema.db, constant_module.into()) {
return None;
}
outer_exists = true;
current_module = m;
}
Some(_) => {
return None;
}
None => {
not_exist_name_ref.push(name_ref_value);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is fine for now, but ideally we want a helper function here that just creates/resolves a path for all its modules, as this here doesn't cover some cases properly.

Comment on lines 90 to 92
if file_id.is_some() {
builder.edit_file(file_id.unwrap());
}
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 file_id.is_some() {
builder.edit_file(file_id.unwrap());
}
if let Some(file_id) = file_id {
builder.edit_file(file_id);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@harpsword harpsword force-pushed the fix_generate_constant branch from 85ffba3 to b5aa3b3 Compare July 18, 2022 00:44
@Veykril
Copy link
Member

Veykril commented Jul 18, 2022

Thanks
@bors r+

@bors
Copy link
Contributor

bors commented Jul 18, 2022

📌 Commit b5aa3b3 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 18, 2022

⌛ Testing commit b5aa3b3 with merge 8e379ce...

@bors
Copy link
Contributor

bors commented Jul 18, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 8e379ce to master...

@bors bors merged commit 8e379ce into rust-lang:master Jul 18, 2022
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.

“Generate constant” ignores the path prefix of the identifier
3 participants