Skip to content

Tag the inserted lifetime bounds with the span of the other bounds #191

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

Closed
wants to merge 1 commit into from

Conversation

notriddle
Copy link

Fixes rust-lang/rust#93828 (which is really a bug in async_trait, not rustc).

Before:

help: consider further restricting this bound
   |
13 |     async fn publish<T + std::marker::Send: IntoUrl>(&self, url: T) -> String {
   |                        +++++++++++++++++++

After:

help: consider further restricting this bound
   |
13 |     async fn publish<T: IntoUrl + std::marker::Send>(&self, url: T) -> String {
   |                                 +++++++++++++++++++

Fixes rust-lang/rust#93828 (which is really a bug in `async_trait`,
not rustc).

Before:

	help: consider further restricting this bound
	   |
	13 |     async fn publish<T + std::marker::Send: IntoUrl>(&self, url: T) -> String {
	   |                        +++++++++++++++++++

After:

	help: consider further restricting this bound
	   |
	13 |     async fn publish<T: IntoUrl + std::marker::Send>(&self, url: T) -> String {
	   |                                 +++++++++++++++++++
@notriddle notriddle force-pushed the notriddle/span-bounds branch from e34bdee to e488f79 Compare March 7, 2022 19:00
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I am hesitant to accept this because I don't think the change in the PR makes things consistently better. Obviously the ui test you gave, with a single trait bound which is a single identifier, gets better with this change. But a bunch of other errors get disproportionately worse:

Before:

   |
15 |     async fn publish<T + std::marker::Send: Trait<u8>>(&self, url: T) {}
   |                        +++++++++++++++++++

After, stable rustc:

   |
15 |     async fn publish<T: Trait + std::marker::Send<u8>>(&self, url: T) {}
   |                               +++++++++++++++++++

After, nightly rustc:

   |
15 |     async fn publish<T: Trait<u8>> + std::marker::Send(&self, url: T) {}
   |                                    +++++++++++++++++++

Also as you wrote in the comment in the ui test, fundamentally this is something that can't be accomplished by hacking around with spans. "It ought to insert the colon, but getting it to do that would require a way to tell rustc that the bounds don't actually exist in the source code, and it needs to insert them."

So I would prefer to see a fix in rustc instead. It could place the + Send suggestion at the max of the spans of all of the type parameter's trait bounds, instead of blindly after the span of the last trait bound. That way macro-generated trait bounds which are spanned with type parameter's span (async-trait is definitely not the only macro that does this) don't wreck the suggestion placement.

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.

Syntactically invalid suggestion for "help: consider further restricting this bound"
2 participants