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

Lament the invincibility of the Turbofish #53562

Merged
merged 1 commit into from
Aug 24, 2018

Conversation

varkor
Copy link
Member

@varkor varkor commented Aug 21, 2018

Here a test case is added to ensure that any others attempting to drive the Turbofish to extinction have second thoughts. Previously the entire test suite would succeed if generic arguments were accepted without disambiguation, making for confusing and heartbreaking circumstances.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 21, 2018
@Centril
Copy link
Contributor

Centril commented Aug 21, 2018

I must say: "oh noes".
Never has there been a sadder day.
Yet for all our woes,
There is great melancholy here, I must say.
The Great Turbofish is here to stay.

// in permitting generic arguments to be provided without the consent of the
// Great Turbofish. Should you be so naïve as to try to revolt against its
// mighty clutches, here shall its wrath be indomitably displayed. This
// program must pass for all eternity, fundamentally at odds with a impetuous
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if there's some sort of poetic device being used here, but to my untrained eye, "a impetuous" seems like an error.

@varkor varkor force-pushed the bastion-of-the-turbofish branch from 6b664fa to 865b76a Compare August 21, 2018 15:41
@llogiq
Copy link
Contributor

llogiq commented Aug 21, 2018

A turbofish, often lamented
and shunned, even called "demented",
has the last laugh this time
so with song and with rhyme
it is hereby rust-astic cemented.

@varkor varkor force-pushed the bastion-of-the-turbofish branch from 865b76a to 7f4442c Compare August 21, 2018 16:26
@nagisa
Copy link
Member

nagisa commented Aug 21, 2018

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 21, 2018

📌 Commit 7f4442c5105f4950cf4c19d37cf39fed5347ee0d has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2018
@wycats
Copy link
Contributor

wycats commented Aug 21, 2018

@varkor While I definitely agree that this makes it "not a slam dunk", I don't think it means that there is no hope. The case in question is very obscure, would be very unlikely to compile both ways in any real scenario, is easily linted (in the case of ambiguity). It is therefore possible that we could decide to make the change anyway (with a long enough time horizon, and with enough practical surveying), and I wouldn't want someone interested in doing the (harder) work of a transition plan to find this issue and decide that there was no point in even attempting it.

@varkor
Copy link
Member Author

varkor commented Aug 21, 2018

@wycats: I personally would love to see a transition plan, though I think the constraints are difficult. The test case here is just to make sure that if someone does intend to tackle the problem, they're aware of this particular ambiguity. I think there's enough context to make clear the point of the test (the origin should be easily located with a quick search).

@withoutboats
Copy link
Contributor

Wouldn't propose any change to this PR, but I agree with @wycats that this isn't necessarily the hard blocker. I would enumerate these constraints on getting rid of the turbofish:

  1. How will we migrate these ambiguous edge cases?
  2. How does it impact our grammatical complexity (both formally and informally)? How much more difficult is it to implement a Rust parser?
  3. How does the implementation impact compile times?

None of these seem impossible to overcome, but possibly difficult.

@eddyb
Copy link
Member

eddyb commented Aug 22, 2018

We can "just" make this test fail in Rust 2018, right? Riiight?

// Bastion of the Turbofish
// ------------------------
// Beware travellers, lest you venture into waters callous and unforgiving,
// where hope must abandoned, ere it is cruelly torn from you. For here stands

Choose a reason for hiding this comment

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

// where hope must *be* abandoned?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nicely caught, thanks!

@varkor varkor force-pushed the bastion-of-the-turbofish branch from 7f4442c to 3d5fef6 Compare August 22, 2018 14:01
@varkor
Copy link
Member Author

varkor commented Aug 22, 2018

@bors r=nagisa rollup

@bors
Copy link
Contributor

bors commented Aug 22, 2018

📌 Commit 3d5fef6 has been approved by nagisa

@KamilaBorowska
Copy link
Contributor

Something worth nothing is that in C# 2.0, a breaking change was introduced to change the way a<b,c>(d) is parsed. I can see this being feasible to do in Rust as well.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 22, 2018
…nagisa

Lament the invincibility of the Turbofish

Here a test case is added to ensure that any others attempting to drive the Turbofish to extinction have second thoughts. Previously the [entire test suite would succeed](rust-lang#53511) if generic arguments were accepted without disambiguation, making for [confusing and heartbreaking circumstances](rust-lang/rfcs#2527).
@mark-i-m
Copy link
Member

@varkor Perhaps the test should include a link to this issue for context?

bors added a commit that referenced this pull request Aug 22, 2018
Rollup of 10 pull requests

Successful merges:

 - #53418 (Mark some suggestions as MachineApplicable)
 - #53431 (Moved some feature gate ui tests to correct location)
 - #53442 (Update version of rls-data used with save-analysis)
 - #53504 (Set applicability for more suggestions.)
 - #53541 (Fix missing impl trait display as ret type)
 - #53544 (Point at the trait argument when using unboxed closure)
 - #53558 (Normalize source line and column numbers.)
 - #53562 (Lament the invincibility of the Turbofish)
 - #53574 (Suggest direct raw-pointer dereference)
 - #53585 (Remove super old comment on function that parses items)

Failed merges:

 - #53472 (Use FxHash{Map,Set} instead of the default Hash{Map,Set} everywhere in rustc.)
 - #53563 (use String::new() instead of String::from(""), "".to_string(), "".to_owned() or "".into())

r? @ghost
@varkor varkor force-pushed the bastion-of-the-turbofish branch from 3d5fef6 to 8e90807 Compare August 22, 2018 20:03
@Centril
Copy link
Contributor

Centril commented Aug 22, 2018

My world is filled with unbounded sorrow.
I feel there is no tomorrow.
All that I had ever wished for,
Burnt down to its core.
But perhaps someone left open the door,
Could there be a solution in store?
For Turbofish to be fixed...
Or will that evermore be nixed?
The opinions seem to be mixed.

@varkor
Copy link
Member Author

varkor commented Aug 22, 2018

@mark-i-m: I added one, but slightly too late for the rollup. We can make the change as a follow-up if need be (though given how much attention has been drawn to it now, I imagine it won't be hard to find if anyone stumbles upon it in the test suite 😄).

@bors
Copy link
Contributor

bors commented Aug 22, 2018

☔ The latest upstream changes (presumably #53607) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 22, 2018
@varkor varkor force-pushed the bastion-of-the-turbofish branch from 8e90807 to b188c2a Compare August 22, 2018 22:25
@varkor
Copy link
Member Author

varkor commented Aug 22, 2018

Are things going to go terribly wrong if I re-approve this after it's been merged as a rollup, with the context clarification? Let's see.

@bors r=nagisa rollup

@bors
Copy link
Contributor

bors commented Aug 22, 2018

📌 Commit b188c2a has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 22, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Aug 22, 2018
…nagisa

Lament the invincibility of the Turbofish

Here a test case is added to ensure that any others attempting to drive the Turbofish to extinction have second thoughts. Previously the [entire test suite would succeed](rust-lang#53511) if generic arguments were accepted without disambiguation, making for [confusing and heartbreaking circumstances](rust-lang/rfcs#2527).
kennytm added a commit to kennytm/rust that referenced this pull request Aug 24, 2018
…nagisa

Lament the invincibility of the Turbofish

Here a test case is added to ensure that any others attempting to drive the Turbofish to extinction have second thoughts. Previously the [entire test suite would succeed](rust-lang#53511) if generic arguments were accepted without disambiguation, making for [confusing and heartbreaking circumstances](rust-lang/rfcs#2527).
bors added a commit that referenced this pull request Aug 24, 2018
Rollup of 16 pull requests

Successful merges:

 - #53311 (Window Mutex: Document that we properly initialize the SRWLock)
 - #53503 (Discourage overuse of mem::forget)
 - #53545 (Fix #50865: ICE on impl-trait returning functions reaching private items)
 - #53559 (add macro check for lint)
 - #53562 (Lament the invincibility of the Turbofish)
 - #53563 (use String::new() instead of String::from(""), "".to_string(), "".to_owned() or "".into())
 - #53592 (docs: minor stylistic changes to str/string docs)
 - #53594 (Update RELEASES.md to include clippy-preview)
 - #53600 (Fix a grammatical mistake in "expected generic arguments" errors)
 - #53614 (update nomicon and book)
 - #53617 (tidy: Stop requiring a license header)
 - #53618 (Add missing fmt examples)
 - #53636 (Prefer `.nth(n)` over `.skip(n).next()`.)
 - #53644 (Use SmallVec for SmallCStr)
 - #53664 (Remove unnecessary closure in rustc_mir/build/mod.rs)
 - #53666 (Added rustc_codegen_llvm to compiler documentation.)
@bors bors merged commit b188c2a into rust-lang:master Aug 24, 2018
@varkor varkor deleted the bastion-of-the-turbofish branch August 24, 2018 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.