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

impl dyn for AnyType fails to compile #50405

Closed
Havvy opened this issue May 3, 2018 · 18 comments
Closed

impl dyn for AnyType fails to compile #50405

Havvy opened this issue May 3, 2018 · 18 comments
Assignees
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition A-parser Area: The parsing of Rust source code to an AST regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-epoch Working group: Epoch (2018) management

Comments

@Havvy
Copy link
Contributor

Havvy commented May 3, 2018

Playpen

fn main() {
    let s = Box::new(S);
    let d = s as Box<dyn dyn>;
    d.say();
}

trait dyn {
    fn say(&self) -> &'static str;
}

struct S;

impl dyn for S {
    fn say(&self) -> &'static str { "I'm S!" }
}

Expected: Program outputs "I'm S!" (missed a println) compiles.

Actual:

Compiling playground v0.0.1 (file:///playground)
error: expected `<`, found `S`
  --> src/main.rs:13:14
   |
13 | impl dyn for S {
   |              ^ expected `<` here

error: aborting due to previous error

error: Could not compile `playground`.

Since dyn is a contextual keyword and still allowed as an identifier, I expect to still be able to implement a trait named "dyn" for various structs. And indeed, if I change the trait name to "dyna", it works.

@Havvy Havvy changed the title impl dyn for S fails to compile impl dyn for AnyType fails to compile May 3, 2018
@retep998 retep998 added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label May 3, 2018
@kennytm
Copy link
Member

kennytm commented May 3, 2018

You could use impl dyn<> for S { } as workaround.

@Havvy
Copy link
Contributor Author

Havvy commented May 3, 2018

Actually, this is failing on stable as well? Uhh....

It's failing in Rust 1.24 (on my computer) and Rust 1.25 (playpen). I don't have older versions of Rust to test with.

Update: Using Godbolt, it compiles on Rust 1.22 and fails to compile on Rust 1.23.

@retep998 retep998 added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels May 3, 2018
@petrochenkov petrochenkov added the A-parser Area: The parsing of Rust source code to an AST label May 3, 2018
@petrochenkov petrochenkov self-assigned this May 3, 2018
@nikomatsakis nikomatsakis added the P-high High priority label May 3, 2018
@nikomatsakis
Copy link
Contributor

triage: P-high

Parsing regression. @petrochenkov seems to be on it =)

Also, @Havvy, you are a bad, bad person for trying to do this. 😛

@pietroalbini pietroalbini added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 4, 2018
@petrochenkov
Copy link
Contributor

We need to increase lookahead to at least 2 tokens to disambiguate (I'll submit a PR soon-ish).
for alone looks too much like a bound start for a poly-trait bound dyn for<'a> Trait<'a>.

But it would still be not enough for precise disambiguation, because impl dyn for <Type as Trait>::AssocTy { ... } is not a dyn Trait.
So we'd need to 3 tokens, but that would be not enough too because impl dyn for <'a + Trait1 as Trait2>::AssocTy { ... } is not a dyn Trait either.

So maybe we shouldn't even start increasing lookahead and keep breakage as is?

@nikomatsakis nikomatsakis added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-nominated labels May 17, 2018
@nikomatsakis
Copy link
Contributor

Nominating for lang team discussion.

@nagisa
Copy link
Member

nagisa commented May 17, 2018

AFAICT this needs infinite look-ahead to work as a contextual keyword.

@Centril Centril removed I-nominated P-high High priority labels May 17, 2018
@Havvy
Copy link
Contributor Author

Havvy commented May 22, 2018

What was the outcome of the lang team discussion?

@nikomatsakis
Copy link
Contributor

Oops! I'll try to leave a more polished comment later :) but the TL;DR was we felt like it was not necessarily worth fixing this.

@nikomatsakis
Copy link
Contributor

Argh. I'm slow. The basic conclusion was that we felt like it was not worth trying to fix this, given the complications that it induces, and given that (as far as we know) no real code in the wild is relying on being able to name a trait dyn (if that last assumption is false, I would like to know). Even if there were such code, presumably it can be rewritten to use r#dyn — a shame to force a rewrite, but it's really an edge case.

However, it occurs to me now in we might want to at least make it a hard error to declare such a trait for clarity. That is, we should not parse trait dyn { } successfully either (and require trait r#dyn).

@nikomatsakis
Copy link
Contributor

Thoughts on this @rust-lang/lang or @petrochenkov ?

However, it occurs to me now in we might want to at least make it a hard error to declare such a trait for clarity. That is, we should not parse trait dyn { } successfully either (and require trait r#dyn).

@nikomatsakis
Copy link
Contributor

Nominating again both to double check on this final point (lang) and to try and assess a priority (compiler or lang).

@withoutboats
Copy link
Contributor

I agree that it makes sense to disallow trait dyn if you can't impl dyn.

@nikomatsakis
Copy link
Contributor

We discussed in the @rust-lang/lang meeting and decided that we should prohibit traits named dyn, full stop, pending any further complications.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 31, 2018

My sense is that this is high priority because of back-compat, hence I am going to mark as P-high. I may or may not prep the PR =) — @petrochenkov maybe you would like to do it?

@nikomatsakis nikomatsakis added the P-high High priority label May 31, 2018
@nikomatsakis nikomatsakis self-assigned this Jun 21, 2018
@nikomatsakis
Copy link
Contributor

OK, we've made no progress here. Assigning to myself, I'll try to whip up a patch.

@nikomatsakis nikomatsakis added WG-epoch Working group: Epoch (2018) management and removed T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jun 28, 2018
@nrc nrc added the A-edition-2018-lints Area: Lints supporting the 2018 edition label Jul 2, 2018
@nrc nrc mentioned this issue Jul 4, 2018
15 tasks
@nikomatsakis
Copy link
Contributor

Almost got this PR prepared. Sorry, kept putting this off. I wound up just banning dyn from being used for the name of any type, for consistency.

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Jul 5, 2018
@nikomatsakis nikomatsakis removed the P-high High priority label Jul 6, 2018
@nikomatsakis
Copy link
Contributor

OK, on the PR discussion, we decided to take a somewhat different tack.

Basically: you can name traits as dyn, but to implement them, you should write impl self::dyn or impl r#dyn. Seems fine.

@nikomatsakis
Copy link
Contributor

I'm going to close this issue then. Thanks to @Havvy for bringing it to our attention. =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition A-parser Area: The parsing of Rust source code to an AST regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-epoch Working group: Epoch (2018) management
Projects
None yet
Development

No branches or pull requests

10 participants