Improved error message for impl on typedef.

[chromatic]

This addresses GH #9767. The text of the error message can, of course, change to meet reviewer preferences.

[huonw]

This branch is only triggered when self_type.ty is a typedef, right? Could the comment be updated to include this information?

[huonw]

BTW, if you write "Fixes #9767" or "Closes #9767" in the commit and/or pull request, github will automatically close that issue when this lands. (There's very little flexibility for the formatting.)

[chromatic]

How about this comment? It's a lot more specific.

[huonw]

That comment looks good, yes.

I don't understand the code fully, but it seems like get_base_type_def_id returns None for primitive types like int and tuples (since it calls get_base_type), so does this cause impl int { ... } to emit the "cannot provide impl for typedefs" error?

Also, what about something like

struct Foo;
type Bar = Foo;
impl Bar { ... }

where the typedef is pointing to a non-primitive?

[chromatic]

Yes, it does--but that case already emitted an error. Only the text of the error message changed in that particular branch.

[alexcrichton]

Are you sure that this branch only happens for typedefs?

[alexcrichton]

I'm very unfamiliar with this code, so I'm mostly just curious

[chromatic]

I'm not 100% positive. All of the existing tests passed, but that's an argument from code coverage. Where would I look to find other potential test cases?

[huonw]

@chromatic I'm not quite sure what you mean by "yes" (since I asked two questions).

[chromatic]

@huonw, Sorry, that was really misleading of me!

impl int { ... } will trigger the error message as you suggested. As for the second question, when the typedef points to something non primitive, you get the error message "cannot associate methods with a type outside the crate the type is defined in; define and implement a trait or new type instead
". That behavior should be unchanged with or without this patch.

[huonw]

Impl-ing directly on primitive types like int should not trigger a message that says "typedefs", and typedefs like my second one should. So, unfortunately, it seems that this patch is not the correct way to implement this.

[chromatic]

This discussion may have identified multiple issues. This list may not be exhaustive, but it's a start:

1. What's the preferred error message for impl-ing a typedef?

2. Should it be possible to impl a primitive type?

2a) If so, is the syntax you gave correct?

2b) If not, what should the error message be? (There's no test for that.)

3. Should it be possible to impl a typedef pointing to a non-primitive?

3a) If so, is the syntax you gave correct?

3b) If not, what should the error message be? (There's no test for that.)

4. Are there other cases where impl-ing something is prohibited?

4a) If so, what should those error messages be? (There are no tests for that.)

5. If the existing code before this PR was in fact incomplete, should this single ticket and PR address all of those cases, or should the additional error cases have their own tickets?

I am very willing to continue addressing this issue, but it seems like these questions need to be answered before the code work can continue.

[huonw]

Most of these questions aren't really brought up by this issue specifically. The primitive/non-primitive distinction essentially only cropped up as an artifact of the implementation you've given us (because the compiler doesn't seem to provide a quick-and-easy way to identify if a type is a type synonym).

1. What's the preferred error message for impl-ing a typedef?

"Cannot impl a type synonym"

2. Should it be possible to impl a primitive type?

Maybe, some loose discussion has happened in a few places (spread across IRC and a few github issues).

2a) If so, is the syntax you gave correct?

Probably.

2b) If not, what should the error message be? (There's no test for that.)

Preferably "Cannot impl directly on a built-in type, use a trait" (or something like that).

3. Should it be possible to impl a typedef pointing to a non-primitive?

I think type synonyms should be handled uniformly, whether or not they point to primitives.

3a) If so, is the syntax you gave correct?

Why would we change from impl Foo?

3b) If not, what should the error message be? (There's no test for that.)

See 1.

4. Are there other cases where impl-ing something is prohibited?

A few, but barely related to type synonyms.

4a) If so, what should those error messages be? (There are no tests for that.)

Eh? There are a few tests for invalid impls.

5. If the existing code before this PR was in fact incomplete, should this single ticket and PR address all of those cases, or should the additional error cases have their own tickets?

This PR is purporting to fix error messages for type synonyms, and so would preferably address every type synonym case (or at least most of them, without regressing other error reporting).

[alexcrichton]

Closing due to a lack of activity, but feel free to reopen with a rebase!