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

Fix auto-application of associated generic functions with placeholders #101990

Merged

Conversation

clubby789
Copy link
Contributor

Fixes #101920

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 18, 2022
@rust-highfive
Copy link
Collaborator

r? @compiler-errors

(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 Sep 18, 2022
@clubby789
Copy link
Contributor Author

I notice that even when the type is known and this isn't an issue (e.g)

fn main() {
        let b = Box::new("abc");
        let ptr = b.into_raw();
}

Applying the fix results in incorrect code: Box::<&str>::into_raw()
Should the fix instead transform this to Box::<&str>::into_raw(b)?

@clubby789 clubby789 changed the title Prevent auto-application of associated functions with placeholders Fix auto-application of associated generic functions with placeholders Sep 22, 2022
@compiler-errors
Copy link
Member

Should the fix instead transform this to Box::<&str>::into_raw(b)?

I think so, yes.

@clubby789
Copy link
Contributor Author

Currently working on automatically adjusting the arguments, but not sure how to handle the case of

struct GenericAssocMethod<T>(T);
impl<T> GenericAssocMethod<T> {
  fn default_hello() {}
}
let x = GenericAssocMethod(33);
x.default_hello();

Where the generic arguments can't be inferred in the GenericAssocMethod::<_>::default_hello form. Applying this or leaving it results in invalid code which causes cargo to print a bug message.

@clubby789
Copy link
Contributor Author

I've temporarily commented out the problematic test case since this seems like incorrect behaviour from Cargo

fn main() {
// Test for inferred types
let x = GenericAssocMethod(33);
// This particular case is unfixable without more information by the user,
Copy link
Member

Choose a reason for hiding this comment

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

Can you be explicit about needing more information? If this isn't a MachineApplicable diagnostic, then it shouldn't be marked MachineApplicable I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The case of x.default_hello() (or any case where the type can't be inferred) emits a HasPlaceholders diagnostic and suggests GenericAssocMethod::<_>::default_hello()

@compiler-errors
Copy link
Member

Where the generic arguments can't be inferred in the GenericAssocMethod::<_>::default_hello form. Applying this or leaving it results in invalid code which causes cargo to print a bug message.

Then this suggestion should probably not be MachineApplicable, and we shouldn't have a test case that has // run-rustfix for it, but just a regular error test case for it.

@compiler-errors
Copy link
Member

Marking this as S-waiting-on-author so it can be adjusted to make it not MachineApplicable if it doesn't actually fit the criteria for MachineApplicable, namely that it should produce code that compiles when applied.

@compiler-errors compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2022
@bors
Copy link
Contributor

bors commented Sep 27, 2022

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

@clubby789 clubby789 force-pushed the dont-machine-apply-placeholder-method branch from 40c6828 to 45f61a2 Compare September 27, 2022 14:24
@clubby789
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 28, 2022
@bors
Copy link
Contributor

bors commented Oct 10, 2022

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

@clubby789 clubby789 force-pushed the dont-machine-apply-placeholder-method branch from 45f61a2 to 6851f75 Compare October 10, 2022 12:29
@rustbot
Copy link
Collaborator

rustbot commented Oct 10, 2022

Some changes occurred in src/tools/cargo

cc @ehuss

@clubby789
Copy link
Contributor Author

🤔 Sorry about the ping, I think I might have made a mistake while rebasing

@clubby789 clubby789 force-pushed the dont-machine-apply-placeholder-method branch from 6851f75 to f490cff Compare October 10, 2022 13:05
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 21, 2022

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

@clubby789 clubby789 force-pushed the dont-machine-apply-placeholder-method branch from 5a19cb1 to 6686074 Compare October 23, 2022 12:16
@bors
Copy link
Contributor

bors commented Nov 4, 2022

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

@clubby789 clubby789 force-pushed the dont-machine-apply-placeholder-method branch from 6686074 to 7df4b0b Compare November 5, 2022 23:08
@compiler-errors
Copy link
Member

@bors r+

@compiler-errors
Copy link
Member

compiler-errors commented Nov 10, 2022

Why didn't bors respond, lol

@bors r+

@bors
Copy link
Contributor

bors commented Nov 10, 2022

📌 Commit 7df4b0b has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 10, 2022
@bors
Copy link
Contributor

bors commented Nov 10, 2022

⌛ Testing commit 7df4b0b with merge 5eef9b2...

@bors
Copy link
Contributor

bors commented Nov 10, 2022

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 5eef9b2 to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Nov 10, 2022

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 5eef9b2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 10, 2022
@bors bors merged commit 5eef9b2 into rust-lang:master Nov 10, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 10, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5eef9b2): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.2% [1.2%, 1.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.6% [-3.5%, -1.3%] 3
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…lder-method, r=compiler-errors

Fix auto-application of associated generic functions with placeholders

Fixes rust-lang#101920
@clubby789 clubby789 deleted the dont-machine-apply-placeholder-method branch February 11, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion for using function call instead of method call syntax incorrectly marked machine applicable
8 participants