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

mem_replace_with_default: recognize some std library ctors #6820

Conversation

mgacek8
Copy link
Contributor

@mgacek8 mgacek8 commented Mar 1, 2021

fixes #6562
changelog: mem_replace_with_default: recognize some common constructors equivalent to Default::default()

@rust-highfive
Copy link

r? @phansch

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 1, 2021
paths::BTREEMAP_NEW.as_ref(),
paths::HASHSET_NEW.as_ref(),
paths::BTREESET_NEW.as_ref(),
paths::BINARY_HEAP_NEW.as_ref(),
Copy link
Contributor

@camsteffen camsteffen Mar 2, 2021

Choose a reason for hiding this comment

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

Suggested change
paths::BINARY_HEAP_NEW.as_ref(),
(sym::BinaryHeap, sym::new),

All of these types will have diagnostic items after the next sync with rustc so we should probably use those instead of adding more paths. You can use the util function is_diagnostic_assoc_item instead of match_def_path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't aware of it, thanks! Makes sense to me to wait then.
I got a question related to this:

  • For Default trait, won't we need default_trait symbol specified (similar to e.g. debug_trait)? It is not defined for the Default trait. Or maybe Default symbol is sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that one needs to be added. Or use the current method for now. The symbol needed just depends on what the diagnostic item is named in #[rustc_diagnostic_item = "name"]. Personally I like naming it the same as the item like Default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that the name we need to use comes from #[rustc_diagnostic_item = "name"], but by that:

Or maybe Default symbol is sufficient?

I meant already defined Default symbol in symbol.rs. I don't quite understand what it is then (because #[rustc_diagnostic_item="..." is not defined for the Default trait)

Copy link
Contributor

@camsteffen camsteffen Mar 4, 2021

Choose a reason for hiding this comment

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

A Symbol is just an interned string. So if we add #[rustc_diagnostic_item="Default"] and Default is already in symbol.rs then we don't need to add to symbol.rs. We also could use sym!(Default) which expands to Symbol::intern("Default"). But having sym::Default is better because it is const. I hope that answers your question?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it is more clear, thanks!
Created this PR in rust repo to add diagnostic item to Default trait.

@bors
Copy link
Contributor

bors commented Mar 7, 2021

☔ The latest upstream changes (presumably #6823) made this pull request unmergeable. Please resolve the merge conflicts.

@phansch phansch added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 8, 2021
@phansch
Copy link
Member

phansch commented Mar 8, 2021

Marking this as S-blocked until we did the sync

@phansch phansch added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work labels Mar 12, 2021
@phansch
Copy link
Member

phansch commented Mar 12, 2021

Hi @mgacek8, the Rustup landed so the diagnostic item for the Default trait should be available in Clippy now =)

@mgacek8 mgacek8 force-pushed the issue_6562_enhance_mem_replace_with_default_with_other_ctors branch from f95b514 to 41be515 Compare March 12, 2021 21:12
@phansch
Copy link
Member

phansch commented Mar 13, 2021

@bors r+ thanks!

@bors
Copy link
Contributor

bors commented Mar 13, 2021

📌 Commit 41be515 has been approved by phansch

@bors
Copy link
Contributor

bors commented Mar 13, 2021

⌛ Testing commit 41be515 with merge 92b9677...

@bors
Copy link
Contributor

bors commented Mar 13, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: phansch
Pushing 92b9677 to master...

@bors bors merged commit 92b9677 into rust-lang:master Mar 13, 2021
@mgacek8 mgacek8 deleted the issue_6562_enhance_mem_replace_with_default_with_other_ctors branch March 20, 2021 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance mem_replace_with_default with other constructors
5 participants