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.

Sign up for GitHub

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

Jump to bottom

Suggest -> impl Trait and -> Box<dyn Trait> on fn that doesn't return #70998

Merged
merged 2 commits into from
Apr 22, 2020
Merged

Suggest -> impl Trait and -> Box<dyn Trait> on fn that doesn't return #70998

merged 2 commits into from
Apr 22, 2020

Conversation

estebank
Copy link
Contributor

During development, a function could have a return type set that is a
bare trait object by accident. We already suggest using either a boxed
trait object or impl Trait if the return paths will allow it. We now
do so too when there are no return paths or they all resolve to !.
We still don't handle cases where the trait object is not the entirety
of the return type gracefully.

Closes #38376.

@rust-highfive
Copy link
Collaborator

r? @varkor

(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 Apr 10, 2020
@estebank estebank mentioned this pull request Apr 10, 2020
Closed
@rust-highfive

This comment has been minimized.

Sign in to view
Centril
Centril reviewed Apr 11, 2020
View reviewed changes
src/librustc_trait_selection/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
src/librustc_trait_selection/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
src/librustc_trait_selection/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
src/librustc_trait_selection/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
src/librustc_trait_selection/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
src/librustc_trait_selection/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
src/librustc_trait_selection/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
src/librustc_trait_selection/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/mod.rs Show resolved Hide resolved
src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.stderr Show resolved Hide resolved
@estebank estebank force-pushed the suggest-impl-trait-empty-fn branch from e53a129 to ea1cdc7 Compare April 14, 2020 03:15
@rust-highfive

This comment has been minimized.

Sign in to view
@estebank estebank force-pushed the suggest-impl-trait-empty-fn branch from ea1cdc7 to adce610 Compare April 14, 2020 04:37
@estebank
Suggest -> impl Trait and -> Box<dyn Trait> on fn that doesn't re…
d3c96f0 
…turn

During development, a function could have a return type set that is a
bare trait object by accident. We already suggest using either a boxed
trait object or `impl Trait` if the return paths will allow it. We now
do so too when there are *no* return paths or they all resolve to `!`.
We still don't handle cases where the trait object is *not* the entirety
of the return type gracefully.
@estebank estebank force-pushed the suggest-impl-trait-empty-fn branch from adce610 to 19fb509 Compare April 20, 2020 16:46
@rust-highfive

This comment has been minimized.

Sign in to view
@varkor
Copy link
Member

varkor commented Apr 20, 2020

@estebank: could you squash the review comment and rustfmt commits?

@estebank
Ensure tail expression will have a Ty for E0746
e536257 
When the return type is `!Sized` we look for all the returned
expressions in the body to fetch their types and provide a reasonable
suggestion. The tail expression of the body is normally evaluated after
checking whether the return type is `Sized`. Changing the order of the
evaluation produces undesirable knock down effects, so we detect the
specific case that newcomers are likely to encounter ,returning a single
bare trait object, and only in that case we evaluate the tail
expression's type so that the suggestion will be accurate.
@estebank estebank force-pushed the suggest-impl-trait-empty-fn branch from 19fb509 to e536257 Compare April 20, 2020 18:17
@estebank
Copy link
Contributor Author

@varkor done.

@varkor
Copy link
Member

varkor commented Apr 20, 2020

r=me when CI passes.

@estebank
Copy link
Contributor Author

@bors r=varkor

1 similar comment
@estebank
Copy link
Contributor Author

@bors r=varkor

@bors
Copy link
Contributor

bors commented Apr 21, 2020

📌 Commit e536257 has been approved by varkor

@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 Apr 21, 2020
@bors
Copy link
Contributor

bors commented Apr 21, 2020

📌 Commit e536257 has been approved by varkor

@JohnTitor JohnTitor mentioned this pull request Apr 22, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 22, 2020
@bors
Auto merge of rust-lang#71410 - JohnTitor:rollup-vh6dut5, r=JohnTitor
2dc5b60 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#70998 (Suggest `-> impl Trait` and `-> Box<dyn Trait>` on fn that doesn't return)
 - rust-lang#71236 (Remove unused rustc_serialize::hex module)
 - rust-lang#71366 (Use assoc int consts3)
 - rust-lang#71372 (Fix #! (shebang) stripping account space issue)
 - rust-lang#71384 (Fix stage0.txt version number comment)
 - rust-lang#71390 (Fix incorrect description of E0690)
 - rust-lang#71399 (Clean up E0554 explanation)

Failed merges:

r? @ghost
@bors bors merged commit 24fb393 into rust-lang:master Apr 22, 2020
@SNCPlay42 SNCPlay42 mentioned this pull request Dec 20, 2020
Closed
@estebank estebank mentioned this pull request Jan 29, 2021
Merged
estebank added a commit to estebank/rust that referenced this pull request Feb 3, 2021
@estebank
Remove incorrect delay_span_bug
ede0a71 
The following code is supposed to compile

```rust
use std::ops::BitOr;

pub trait IntWrapper {
    type InternalStorage;
}

impl<T> BitOr for dyn IntWrapper<InternalStorage = T>
where
    Self: Sized,
    T: BitOr + BitOr<Output = T>,
{
    type Output = Self;
    fn bitor(self, _other: Self) -> Self {
        todo!()
    }
}
```

Before this change it would ICE. In rust-lang#70998 the removed logic was added
to provide better suggestions, and the `delay_span_bug` guard was added
to  protect against a potential logic error when returning traits. As it
happens, there are cases, like the one above, where traits can indeed be
returned, so valid code was being rejected.

Fix rust-lang#80207.
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 3, 2021
@m-ou-se
Rollup merge of rust-lang#81532 - estebank:ice-ice-baby, r=pnkfelix
6695944 
Remove incorrect `delay_span_bug`

The following code is supposed to compile

```rust
use std::ops::BitOr;

pub trait IntWrapper {
    type InternalStorage;
}

impl<T> BitOr for dyn IntWrapper<InternalStorage = T>
where
    Self: Sized,
    T: BitOr + BitOr<Output = T>,
{
    type Output = Self;
    fn bitor(self, _other: Self) -> Self {
        todo!()
    }
}
```

Before this change it would ICE. In rust-lang#70998 the removed logic was added
to provide better suggestions, and the `delay_span_bug` guard was added
to  protect against a potential logic error when returning traits. As it
happens, there are cases, like the one above, where traits can indeed be
returned, so valid code was being rejected.

Fix (but not close) rust-lang#80207.
ehuss pushed a commit to ehuss/rust that referenced this pull request Feb 5, 2021
@m-ou-se @ehuss
Rollup merge of rust-lang#81532 - estebank:ice-ice-baby, r=pnkfelix
f6b8e2b 
Remove incorrect `delay_span_bug`

The following code is supposed to compile

```rust
use std::ops::BitOr;

pub trait IntWrapper {
    type InternalStorage;
}

impl<T> BitOr for dyn IntWrapper<InternalStorage = T>
where
    Self: Sized,
    T: BitOr + BitOr<Output = T>,
{
    type Output = Self;
    fn bitor(self, _other: Self) -> Self {
        todo!()
    }
}
```

Before this change it would ICE. In rust-lang#70998 the removed logic was added
to provide better suggestions, and the `delay_span_bug` guard was added
to  protect against a potential logic error when returning traits. As it
happens, there are cases, like the one above, where traits can indeed be
returned, so valid code was being rejected.

Fix (but not close) rust-lang#80207.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centril Centril Centril left review comments

@varkor varkor varkor left review comments

Assignees

@varkor varkor

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Special errors for common trait object issues
5 participants
@estebank @rust-highfive @varkor @bors @Centril