Do not permit type parameters on builtin bounds. #21549
Conversation
| // except according to those terms. | ||
|
|
||
| fn foo<T:Copy<U>, U>(x: T) { | ||
| //~^ ERROR: wrong number of type arguments: expected 0 found 1 |
There was a problem hiding this comment.
is that span correct? shouldn't it be on the Copy or just on the ?
There was a problem hiding this comment.
The ERROR match doesn't check on span positions. The ^ means the line above (L12) should raise this error. You can just remove the spaces in this ERROR line and it'll just work.
There was a problem hiding this comment.
i don't know how i misread that :/
|
Other than that tiny nit about the error message, this looks good to me. @nikomatsakis thoughts on the error message? |
|
Also, great work and thanks for the patch. 🍰 |
| // option. This file may not be copied, modified, or distributed | ||
| // except according to those terms. | ||
|
|
||
| fn foo<T:Copy<U>, U>(x: T) { |
There was a problem hiding this comment.
You may also want some other tests for things like:
trait Trait: Copy<Send> {}
struct MyStruct<T: Copy<T>>;
|
Ok, changed the message and added the tests. |
| if parameters.is_empty() { | ||
| continue; // success | ||
| } | ||
| span_fatal!(tcx.sess, b.trait_ref.path.span, E0316, |
There was a problem hiding this comment.
this should be span_err, not span_fatal
There was a problem hiding this comment.
mmh, do you think we should make the other span_fatal! errors too? We currently consider this kind of errors as fatal errors for other types.
There was a problem hiding this comment.
@ahmedcharles when changing this to span_error you should be able to merge the tests in a single test file.
There was a problem hiding this comment.
In general, every span_fatal is a bug imo, though sometimes there are good reasons. In this particular case, I think those other errors are holdovers and could be made span_err with some effort. (span_err can be trickier because you have to have some kind of intelligent fallback behavior...)
There was a problem hiding this comment.
I fully agree with you. @ahmedcharles if you want to take a stab and make the other span_fatal! errors, it'd be great. Otherwise, I'll do it in a separate patch. :)
There was a problem hiding this comment.
I don't mind changing them, though I'd rather do it in a separate patch.
|
Sorry for the delay! I forgot about this PR since I'm accustomed to just skimming what's assigned to me. :( Assigning to myself. |
|
@ahmedcharles ping, this looks almost ready to go, any chance you can address the various nits? |
|
I've been busy this week and haven't had time to figure out the issues I've had with the tests that were suggested, some of them seem to produce multiple errors, instead of just the one I added. I need some more time to investigate. |
|
@ahmedcharles no worries at all. If you feel you won't have time, we can take the patch over and add the tests ourselves in a separate commit. Also, I wonder if you read my last comment. I explained there what the issues with the tests you tried were. Let me know! |
|
@ahmedcharles hey there, do you think you'll have time to look at this pull request again? If not, one of us can happily take over the patch for you; just let us know so we don't duplicate effort. (I assume if we don't hear back, then we'll just take the patch over.) |
|
I'll either have it fixed today or pass it off. |
|
I'm not sure how to get the tests passing, but the current diff builds and mostly works as expected. |
|
I spoke with @ahmedcharles on IRC, I'll take this over and fix the tests preserving the existing commits. |
|
superseded by #22680 |
Fixes #20302.
r? @flaper87