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

Disable parsing of type ascription in beta and stable #48016

Closed
estebank opened this issue Feb 5, 2018 · 26 comments
Closed

Disable parsing of type ascription in beta and stable #48016

estebank opened this issue Feb 5, 2018 · 26 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@estebank
Copy link
Contributor

estebank commented Feb 5, 2018

Type ascription is a nightly only feature. Due to its syntax, this feature can cause some confusing error messages. Some of these have been handled by checking against typos (like when meaning ;), but given that the feature is only available in nightly, I think we should gate the feature at the parser level as well, so that error messages do not even mention type ascription. The big draw backs of doing this is that once the feature gets stabilized it won't have good diagnostics as it would otherwise have (because they'd only be shown in nightly, so we'd have fewer bug reports about it) and if somebody tries to use a nightly only crate that uses type ascriptions, the compiler would complain about syntax errors, instead of the feature being nightly only.

Having said that, there might be a middle point, where the feature is still being probed for diagnostic purposes, but never actually parsed on its entirety. At the same time, as pointed above, it probably would be worth to instead improve the diagnostics involving type ascription. If that is the decision, this issue can be closed.

cc @rust-lang/compiler

@estebank estebank added A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 5, 2018
@durka
Copy link
Contributor

durka commented Feb 5, 2018

First thought: this sounds like it might lead to odd behavior on nightly. Generally, nightly behaves the same as stable/beta unless you turn on a feature flag (and if you do something that is gated it tells you which feature flag to use). If nightly parses type ascription but gates it (current behavior), while stable/beta don't parse it at all, that's a behavior difference with no feature flag. But if nightly-without-feature-flag doesn't parse the syntax, there's no way to discover the flag. Maybe you'd have to run the parser in both modes, and say "warning: this parses differently with #![feature(type_ascription)]"?

Second thought: would this affect macros? When type ascription was implemented it changed the meaning of macros such as ($e:expr) => {}; ($n:ident : $t:ty) => {}. This sounds like it might end up changing that back, on stable/beta?

So this seems like maybe a good idea to improve error messages but would have to be done really carefully.

@nikomatsakis
Copy link
Contributor

I'm kind of tempted to just rip out type ascription, maybe try again some other time.

@eddyb
Copy link
Member

eddyb commented Feb 5, 2018

GitHub needs 😞/😭 reactions.

@nikomatsakis
Copy link
Contributor

I confess I've never really wanted it though =) I'm happy to be persuaded to keep it...

@durka
Copy link
Contributor

durka commented Feb 5, 2018 via email

@estebank
Copy link
Contributor Author

estebank commented Feb 5, 2018

It is not my intention to completely rip out the feature, but rather to acknowledge that the feature has introduced difficulty to learn the language an significantly bad in such a way that is not actionable for stable/beta users. If the syntax is ever to be stabilized, the learnability and diagnostic problems will need to be addressed, but as it stands now, I feel there are better places to focus on.

As for @durka's comments, they are very valid points.

@nikomatsakis
Copy link
Contributor

I mean it went through the RFC process, seems weird to simply drop it without another one.

(I didn't mean we would drop it without some sort of FCP. In part, I am interested in culling "minor" features that have niggling issues that don't seem worth resolving at the moment.)

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 5, 2018

@estebank

Due to its syntax, this feature can cause some confusing error messages.

Can you elaborate on these? I think that'd be useful information for the feature stabilization process in general.

@cramertj
Copy link
Member

cramertj commented Feb 5, 2018

@nikomatsakis This is the one I see all the time:

error: type ascription is experimental (see issue #23416)
 --> src/main.rs:2:13
  |
2 |     let _ = Box:new();
  |             ^^^^^^^^^

@estebank
Copy link
Contributor Author

estebank commented Feb 5, 2018

@nikomatsakis a few other that I've seen recently:

#39126
#37509
#34255
#47360

@nikomatsakis
Copy link
Contributor

Thanks!

A middle ground, I suppose, would be to generate a better error, right? That might be a pain to do in this case though.

@TimNN TimNN added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Feb 6, 2018
@estebank
Copy link
Contributor Author

estebank commented Feb 6, 2018

The only reason I'm proposing this is due to the multiple places these errors pop up in that need to be handled. Then again, at some point it'll have to be dealt with. The reasoning behind this proposal is that in the meantime stable users instantly get better errors while we improve them in nightly (and added beta just to make sure we had an option to catch regressions that wouldn't come up in nightly due to having different behavior to stable).

@saethlin
Copy link
Member

saethlin commented Mar 4, 2018

@estebank started a thread on internals.rust-lang.org asking for compiler error pet peeves, and I'd just like to mention that the issue being discussed here is one of mine, especially as I just ran into it again today and was scratching my head for a few minutes. As a minimal example for what I just ran into:

fn main() {
    std:io::stdin();
}

I get three errors, two of which mention type ascription. I just wanted to lend my support to cleaning up the compiler messages in this situation.

@nikomatsakis
Copy link
Contributor

cc @rust-lang/lang -- it's worth trying to get some consensus around type ascription. I think that some of us at least regard type ascription as a kind of "not worth it" feature, at least with its current syntax, that introduces a variety of complications for relatively little gain.

@cramertj
Copy link
Member

cramertj commented Mar 6, 2018

@nikomatsakis I'd be +1 for removing it if it helps us give better error messages. If the feature ever stabilizes, it certainly won't be soon, so IMO it's not worth additional effort to keep dragging it along.

@nrc
Copy link
Member

nrc commented Mar 6, 2018

I'm strongly in favour of keeping and stabilising type ascription (although there is the open question about coercions, which has blocked stabilisation). I'm still in favour of landing this PR - seems like it is a win to not tell users about something they can't use.

Reasons I like type ascription:

  • primarily, I find it really useful when trying to debug type inference issues. This never shows up in committed code because it is a temporary use, but I use this fairly often and it is very useful.
  • occasionally it is useful as a less ugly version of turbofish (which I regard as an abomination)
  • for creating DSTs, though this is not currently practical because type ascription doesn't coerce and DSTs are not that common anyway

To head off discussion on the syntax, using a colon is pretty much perfect for type ascription since we use it for type declarations (and it is used for type ascription in other languages). I actually get sad in the opposite direction: when we use colons for anything other than indicating type (or bounds), e.g., I regard using colons for field initialisation as one of our larger syntactic blunders and I'm sad we couldn't change that pre-1.0.

@scottmcm
Copy link
Member

scottmcm commented Mar 7, 2018

A post about a case where "the way to go is to replace casts with coercions", which might be another case for ascription: #46906 (comment)

Given that I want to kill as (once we have sufficient replacements), I'd really like to keep some kind of ascription. While I survived boost::implicit_cast<T> in C++, I'd like to do better, but it feels like a library solution would want to be .coerce::<T>(), which we probably wouldn't do since it would need adding to everything, and suggesting turbofish never really makes me happy either.

@nikomatsakis
Copy link
Contributor

Heh, I just hit this error and it confused me for a good 5 minutes (I had ty::ParamEnv:reveal_all()).

@nikomatsakis
Copy link
Contributor

In general, I am still vaguely against the current type ascription syntax. I agree that : is used for types and that is nice, but I think it has a number of other strikes against it:

  • In general, have expressing forms (like <expr> as <Ty> and <exor>: <ty>) that end in types is annoying from a parsing perspective. It leads to conflicts around the <> operators (e.g., foo as vec < u32 .... -- quick, which way does it parse? I don't remember.)
  • Type ascription in general mixes poorly with explicit ref bindings, but the syntax of expr: Ty makes it look like an lvalue, so we can't just uniformly make it a coercion and make those problems go away.

Nonetheless I agree with @nrc's motivations, though I am less negative about turbofish than he is (and I sort of would prefr that if we hate it so much that we solve it more directly, though maybe there's no hope).

I wonder about repurposing as as a prefix operator:

as<T>(expr)

with the semantics of it being always an rvalue, thus side-stepping the coercion soundness problems.

So let x = as<u32>(22)?

Maybe it's confusing that as wouldn't have to use turbofish when other things must. And of course as::<u32>(22) wouldn't solve nrc's "no turbofish" constraint.

@nikomatsakis
Copy link
Contributor

(also, given that as as I wrote is basically equivalent to:

fn cast<T>(x: T) -> T { x }

it seems sort of silly to bake it in the language. Though I do wonder if we could replace type ascripton with putting that function into the prelude.)

@durka
Copy link
Contributor

durka commented Mar 9, 2018

That function has other uses as well.

@scottmcm
Copy link
Member

scottmcm commented Mar 9, 2018

And there's rust-lang/rfcs#2306 to add it as convert::identity.

I do like that a name related to type ascription gives it more of an obvious purpose, since my first reaction to seeing identity is "why did you need to specify that?". For example drop, coerce, and moving could all be implemented fine as an identity function, but different names help the intent. (And could even have lints like always turbofish cast, but never turbofish moving.)

@durka
Copy link
Contributor

durka commented Mar 9, 2018 via email

@scottmcm
Copy link
Member

scottmcm commented Mar 9, 2018

@durka I'm hoping we get to the world where anything that's not a coercion is accessible through a method or function with clearer semantics. For example, from, try_from, and new wrapping_from (bikeshedded, of course) for going between all the integer types.

@estebank
Copy link
Contributor Author

I believe that #59150 covers a lot of ground of what disabling was meant to accomplish, although there are still cases where errors are not great, namely the following three:

warning: unnecessary path disambiguator
 --> file2.rs:3:36
  |
3 |     println!("{}", std::mem:size_of::<BTreeMap<u32, u32>>());
  |                                    ^^ try removing `::`

error: expected token: `,`
 --> file2.rs:3:58
  |
3 |     println!("{}", std::mem:size_of::<BTreeMap<u32, u32>>());
  |                                                          ^
error: expected type, found keyword `box`
 --> file5.rs:2:25
  |
2 |     let _ = Option:Some(vec![0, 1]);
  |                         ^^^^^^^^^^ in this macro invocation
  |
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error[E0425]: cannot find value `foo` in this scope
 --> file4.rs:5:9
  |
5 |         foo: Vec::new()
  |         ^^^
  |         |
  |         `self` value is a keyword only available in methods with `self` parameter
  |         help: try: `self.foo`

error: parenthesized type parameters may only be used with a `Fn` trait
 --> file4.rs:5:22
  |
5 |         foo: Vec::new()
  |                      ^^
  |
  = note: #[deny(parenthesized_params_in_types_and_modules)] on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #42238 <https://github.com/rust-lang/rust/issues/42238>

error[E0107]: wrong number of type arguments: expected 1, found 0
 --> file4.rs:5:14
  |
5 |         foo: Vec::new()
  |              ^^^^^^^^^^ expected 1 type argument

Centril added a commit to Centril/rust that referenced this issue Mar 26, 2019
Centril added a commit to Centril/rust that referenced this issue Mar 26, 2019
Centril added a commit to Centril/rust that referenced this issue Mar 26, 2019
@estebank
Copy link
Contributor Author

estebank commented Jul 19, 2019

Now with #62791 the only two cases left are the last two in the previous comment. Those are tracked in #47666 and #34255 respectively. For the macro one a fix will require carrying state around from the Parser to the MacroExpander, while for the latter I believe that code is so close to real code that the parser should attempt to parse it outright (for diagnostic purposes). Given this, the raison d'être for this issue is no longer pressing. The rest of the discussion can continue in the tracking issue #23416. Closing.

Centril added a commit to Centril/rust that referenced this issue Jul 21, 2019
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants