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 syntax error in suggesting generic constraint in trait parameter #76695

Merged
merged 2 commits into from
Sep 16, 2020

Conversation

iximeow
Copy link
Contributor

@iximeow iximeow commented Sep 14, 2020

suggest where T: Foo for the first bound on a trait, then suggest
, T: Foo when the suggested bound would add to an existing set of
where clauses. where T: Foo may be the first bound if T has a
default, because we'd rather suggest

trait A<T=()> where T: Copy

than

trait A<T: Copy=()>

for legibility reasons.

the test case i added here is derived from this reproduction:

struct B<T: Copy> {
    t: T
}

trait A<T = ()> {
    fn returns_constrained_type(&self, t: T) -> B<T> {
        B { t }
    }
}

where the suggested fix,

trait A<T = ()>, T: Copy { ... }

is in fact invalid syntax!

i also found an error in the existing suggestion for trait Base<T = String>: Super<T> where rustc would suggest trait Base<T = String>: Super<T>, T: Copy, but T: Copy is the first of the trait's where clauses and should be where T: Copy as well. the test for that suggestion expects invalid syntax, and has been revised to a compiler-pleasing trait Base<T = String>: Super<T> where T: Copy.

judging by #70009 i'll.. cc @estebank ?

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(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 14, 2020
suggest `where T: Foo` for the first bound on a trait, then suggest
`, T: Foo` when the suggested bound would add to an existing set of
`where` clauses. `where T: Foo` may be the first bound if `T` has a
default, because we'd rather suggest
```
trait A<T=()> where T: Copy
```
than
```
trait A<T: Copy=()>
```
for legibility reasons.
@iximeow iximeow force-pushed the trait-generic-bound-suggestion branch from dcd571c to 0eac38b Compare September 14, 2020 04:24
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

Minor wording changes to comments, otherwise r=me!

compiler/rustc_middle/src/ty/diagnostics.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/diagnostics.rs Outdated Show resolved Hide resolved
Co-authored-by: Esteban Küber <esteban@kuber.com.ar>
@iximeow
Copy link
Contributor Author

iximeow commented Sep 14, 2020

Good catches! tweaks applied.

@estebank
Copy link
Contributor

r? @estebank
@bors r+

@bors
Copy link
Contributor

bors commented Sep 14, 2020

📌 Commit e1607c8 has been approved by estebank

@rust-highfive rust-highfive assigned estebank and unassigned oli-obk Sep 14, 2020
@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 Sep 14, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 16, 2020
…ion, r=estebank

fix syntax error in suggesting generic constraint in trait parameter

suggest `where T: Foo` for the first bound on a trait, then suggest
`, T: Foo` when the suggested bound would add to an existing set of
`where` clauses. `where T: Foo` may be the first bound if `T` has a
default, because we'd rather suggest
```
trait A<T=()> where T: Copy
```
than
```
trait A<T: Copy=()>
```
for legibility reasons.

the test case i added here is derived from [this reproduction](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=0bf3ace9f2a183d0bdbd748c6b8e3971):
```
struct B<T: Copy> {
    t: T
}

trait A<T = ()> {
    fn returns_constrained_type(&self, t: T) -> B<T> {
        B { t }
    }
}
```
where the suggested fix,
```
trait A<T = ()>, T: Copy { ... }
```
is in fact invalid syntax!

i also found an error in the existing suggestion for `trait Base<T = String>: Super<T>` where rustc would suggest `trait Base<T = String>: Super<T>, T: Copy`, but `T: Copy` is the first of the trait's `where` clauses and should be `where T: Copy` as well. the test for that suggestion expects invalid syntax, and has been revised to a compiler-pleasing `trait Base<T = String>: Super<T> where T: Copy`.

judging by rust-lang#70009 i'll.. cc @estebank ?
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 16, 2020
Rollup of 10 pull requests

Successful merges:

 - rust-lang#76669 (Prefer asm! over llvm_asm! in core)
 - rust-lang#76675 (Small improvements to asm documentation)
 - rust-lang#76681 (remove orphaned files)
 - rust-lang#76694 (Introduce a PartitioningCx struct)
 - rust-lang#76695 (fix syntax error in suggesting generic constraint in trait parameter)
 - rust-lang#76699 (improve const infer error)
 - rust-lang#76707 (Simplify iter flatten struct doc)
 - rust-lang#76710 (:arrow_up: rust-analyzer)
 - rust-lang#76714 (Small docs improvements)
 - rust-lang#76717 (Fix generating rustc docs with non-default lib directory.)

Failed merges:

r? `@ghost`
@bors bors merged commit 54d7728 into rust-lang:master Sep 16, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants