Remove type ascription from parser and diagnostics #109128

chenyukang · 2023-03-14T16:25:59Z

Mostly based on #106826

Part of #101728

r? @estebank

rustbot · 2023-03-14T16:26:09Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

chenyukang · 2023-03-14T16:26:21Z

ping @Nilstrieb

src/tools/clippy/clippy_utils/src/sugg.rs

rustbot · 2023-03-14T23:11:15Z

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

src/tools/rustfmt/tests/source/type-ascription.rs

calebcartwright · 2023-03-15T17:03:07Z

Have not had a chance to review in detail, but given the changes to expected output of formatting tests in d0b2fc96f919dd4a923ce5ea7538a82fec5e2bdc I want t-rustfmt to have an opportunity to review before merging to ensure the currently proposed changes in this PR do not contain something that would violate rustfmt's formatting stability guarantee

chenyukang · 2023-03-15T17:17:54Z

Have not had a chance to review in detail, but given the changes to expected output of formatting tests in d0b2fc9 I want t-rustfmt to have an opportunity to review before merging to ensure the currently proposed changes in this PR do not contain something that would violate rustfmt's formatting stability guarantee

sure, I checked the diff, and we can not support macro with ascription syntax now.

calebcartwright · 2023-03-15T17:26:48Z

Have not had a chance to review in detail, but given the changes to expected output of formatting tests in d0b2fc9 I want t-rustfmt to have an opportunity to review before merging to ensure the currently proposed changes in this PR do not contain something that would violate rustfmt's formatting stability guarantee

sure, I checked the diff, and we can not support macro with ascription syntax now.

Understood, and don't mean to imply there's any issues. Just leaving a note so that this doesn't get merged before we get an opportunity to review

chenyukang · 2023-03-15T23:46:02Z

@calebcartwright @jyn514

I don't have much experience with rustfmt, I met some issues when resolving #109128 (comment).

how to run the rustfmt which built in stage1?
I run it directly after building:

./build/aarch64-apple-darwin/stage1-tools-bin/rustfmt

I got this error:

dyld[10473]: Library not loaded: @rpath/libstd-8c92ab8e515128e4.dylib
  Referenced from: <2F10C189-2C7F-3447-943C-6509EEC9465A> /Users/yukang/rust/build/aarch64-apple-darwin/stage1-tools-bin/rustfmt
  Reason: tried: '/Users/yukang/rust/build/aarch64-apple-darwin/stage1/lib/libstd-8c92ab8e515128e4.dylib' (no such file), .......
[1]    10473 abort      ./build/aarch64-apple-darwin/stage1-tools-bin/rustfmt

I found the file libstd-8c92ab8e515128e4.dylib located at:

.//build/aarch64-apple-darwin/stage1-std/aarch64-apple-darwin/release/deps/libstd-8c92ab8e515128e4.dylib

and I workaround it in this naive way:

export DYLD_LIBRARY_PATH="$(rustc +dev --print sysroot)/lib"
cp .//build/aarch64-apple-darwin/stage1-std/aarch64-apple-darwin/release/deps/libstd-8c92ab8e515128e4.dylib /Users/yukang/rust/build/aarch64-apple-darwin/stage1/lib

I guess there may be a right way for it?
something like this command to link a dev rustfmt:

rustup component add rustfmt

jyn514 · 2023-03-15T23:50:32Z

@chenyukang there's not a supported way to do this today: #102010

bors · 2023-03-31T20:45:22Z

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

calebcartwright · 2023-04-22T20:27:39Z

Have not had a chance to review in detail, but given the changes to expected output of formatting tests in d0b2fc9 I want t-rustfmt to have an opportunity to review before merging to ensure the currently proposed changes in this PR do not contain something that would violate rustfmt's formatting stability guarantee

sure, I checked the diff, and we can not support macro with ascription syntax now.

Understood, and don't mean to imply there's any issues. Just leaving a note so that this doesn't get merged before we get an opportunity to review

Apologies for the late follow up but no concerns from my end wrt rustfmt changes. The diff looked odd at first blush but the reasoning became clear after refreshing my memory on part of the test contents that weren't part of the diff.

estebank · 2023-04-27T14:53:59Z

tests/ui/type/type-ascription-precedence.stderr

+error: expected one of `!`, `.`, `::`, `;`, `?`, `{`, `}`, or an operator, found `:`
+  --> $DIR/type-ascription-precedence.rs:27:7
   |
-LL |     &(S: &S);
-   |       ^ expected `&S`, found `S`
+LL |     &S: &S;
+   |       ^ expected one of 8 possible tokens


I wonder if it makes sense to leave enough logic in the parser to detect at least :: -> : and ;-> : typos (the later is caught in the test below, but it isn't triggering here, maybe because it is in the same line?). We might also want to leave enough for type ascription parsing with a hard error saying "this used to be parsed but it no longer is" but that is much lower priority (so we can disregard my thought there).

I think we can disregard this, the other positive tests are catching it, so we should be ok.

estebank · 2023-04-27T14:57:50Z

tests/ui/suggestions/type-ascription-instead-of-variant.stderr

   |
-   = note: `#![feature(type_ascription)]` lets you annotate an expression with a type: `<expr>: <type>`
+   = note: type ascription syntax has been removed, see issue #101728 <https://github.com/rust-lang/rust/issues/101728>


Should we soften the wording here with something like "if you meant to annotate an expression with a type, the type ascription syntax has been removed". Someone who encounters the note without knowing what it was might be left a bit baffled.

estebank · 2023-04-27T14:59:22Z

tests/ui/suggestions/type-ascription-instead-of-let.stderr

+error: expected identifier, found `:`
+  --> $DIR/type-ascription-instead-of-let.rs:5:13
   |
 LL |         temp: i32 = fun(5i32);
-   |         ^^^^
-   |         |
-   |         not found in this scope
-   |         help: maybe you meant to write an assignment here: `let temp`
+   |             ^ expected identifier


Weird that we don't suggest let here :-/

The current

help: maybe you meant to write an assignment here: `let temp`

is produced at here:

rust/compiler/rustc_resolve/src/late/diagnostics.rs

Line 1873 in 1a6ae3d

"maybe you meant to write an assignment here",

but the function type_ascription_suggestion is removed.

So it's a preexisting issue, now we only support this scenario:

4 | a = 1; | ^ | help: you might have meant to introduce a new binding | 4 | let a = 1; | +++

I think we should extend to the scenario of a: i32 = 1, I will fix it in another PR.

estebank · 2023-04-27T15:01:17Z

tests/ui/parser/issues/issue-44406.stderr

@@ -29,5 +18,5 @@ LL -         bar(baz: $rest)
 LL +         bar(: $rest)


Preexisting, note to self: this suggestion is broken

bors · 2023-05-02T06:10:49Z

📌 Commit 5d1796a has been approved by iffy

It is now in the queue for this repository.

Dylan-DPC · 2023-05-02T06:11:05Z

@bors rollup=iffy

aaaa...

Dylan-DPC · 2023-05-02T06:12:00Z

@bors r=estebank
sorry for the mess :P

bors · 2023-05-02T06:12:02Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Don't validate constants in const propagation #109521

bors · 2023-05-02T06:12:03Z

📌 Commit 5d1796a has been approved by estebank

It is now in the queue for this repository.

bors · 2023-05-02T09:41:39Z

⌛ Testing commit 5d1796a with merge 98c33e4...

bors · 2023-05-02T13:03:06Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 98c33e4 to master...

rust-timer · 2023-05-02T14:31:45Z

Finished benchmarking commit (98c33e4): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.2%, 0.4%]	9
Regressions ❌ (secondary)	0.3%	[0.2%, 0.5%]	9
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.4%	[-1.4%, -1.4%]	1
All ❌✅ (primary)	0.3%	[0.2%, 0.4%]	9

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.8%	[1.8%, 1.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-4.4%, -2.1%]	5
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.7%	[0.7%, 0.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 655.687s -> 656.873s (0.18%)

Noratrieb · 2023-05-02T16:26:31Z

@estebank can finally be in peace 😌
thank you!

pnkfelix · 2023-05-03T04:43:40Z

primary regressions are to serde, hyper, diesel; mostly to incr-unchanged scenarios.
surprising that this had this impact.
i'm guessing this is noise (this change should be solely simplifying the compiler, AFAICT).
marking as triaged.

@rustbot label: +perf-regression-triaged

…tion, r=estebank Remove type ascription from parser and diagnostics Mostly based on rust-lang#106826 Part of rust-lang#101728 r? `@estebank`

…ompiler-errors Suggest struct when we get colon in fileds in enum A follow-up fix for rust-lang#109128 From: rust-lang#109128 (comment) r? `@estebank`

…ilstrieb Suggest let for possible binding with ty Origin from rust-lang#109128 (comment) r? `@Nilstrieb`

…tion, r=estebank Remove type ascription from parser and diagnostics Mostly based on rust-lang#106826 Part of rust-lang#101728 r? `@estebank`

rustbot assigned estebank Mar 14, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 14, 2023

chenyukang commented Mar 14, 2023

View reviewed changes

src/tools/clippy/clippy_utils/src/sugg.rs Show resolved Hide resolved

chenyukang force-pushed the yukang/remove-type-ascription branch 2 times, most recently from 3ecc00e to 466fb66 Compare March 14, 2023 23:26

This comment has been minimized.

Sign in to view

calebcartwright reviewed Mar 15, 2023

View reviewed changes

src/tools/rustfmt/tests/source/type-ascription.rs Show resolved Hide resolved

chenyukang force-pushed the yukang/remove-type-ascription branch from 466fb66 to 0ac6fae Compare March 15, 2023 01:38

This comment has been minimized.

Sign in to view

chenyukang force-pushed the yukang/remove-type-ascription branch from 0ac6fae to 83bb0e0 Compare March 15, 2023 06:58

This comment has been minimized.

Sign in to view

chenyukang force-pushed the yukang/remove-type-ascription branch 2 times, most recently from a4f74c6 to d0b2fc9 Compare March 15, 2023 16:58

This comment has been minimized.

Sign in to view

chenyukang force-pushed the yukang/remove-type-ascription branch from 0e8b6cd to dbf9825 Compare April 4, 2023 03:37

estebank reviewed Apr 27, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

bors added the merged-by-bors This PR was explicitly merged by bors. label May 2, 2023

bors merged commit 98c33e4 into rust-lang:master May 2, 2023

rustbot added this to the 1.71.0 milestone May 2, 2023

This was referenced May 2, 2023

give some suggestion for returning anonymous enum #100988

Closed

Remember names of cfg-ed out items to mention them in diagnostics #109005

Merged

Do not provide suggestions when the spans come from expanded code that doesn't point at user code #109082

Closed

rustbot added the perf-regression Performance regression. label May 2, 2023

xFrednet mentioned this pull request May 2, 2023

Bump nightly -> 2023-06-01 rust-marker/marker#128

Closed

This was referenced May 3, 2023

Suggest struct when we get colon in fileds in enum #111118

Merged

Suggest let for possible binding with ty #111120

Merged

fmease mentioned this pull request May 3, 2023

Confusing errors for call_method(arg_name: value) when value is not a literal #105810

Closed

rustbot added the perf-regression-triaged The performance regression has been triaged. label May 3, 2023

Noratrieb mentioned this pull request May 7, 2023

remove leftover type_ascription remains in feature gating #111325

Closed

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 9, 2023

Rollup merge of rust-lang#111120 - chenyukang:yukang-suggest-let, r=N…

8c51701

…ilstrieb Suggest let for possible binding with ty Origin from rust-lang#109128 (comment) r? `@Nilstrieb`

notriddle mentioned this pull request Oct 4, 2023

Detect missing => after match guard during parsing #116400

Merged

Noratrieb mentioned this pull request May 2, 2024

Type ascription builtin is silently replaced by rustfmt(?) rust-lang/rustfmt#6159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove type ascription from parser and diagnostics #109128

Remove type ascription from parser and diagnostics #109128

chenyukang commented Mar 14, 2023

rustbot commented Mar 14, 2023

chenyukang commented Mar 14, 2023

rustbot commented Mar 14, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

calebcartwright commented Mar 15, 2023

chenyukang commented Mar 15, 2023

calebcartwright commented Mar 15, 2023

This comment has been minimized.

chenyukang commented Mar 15, 2023 •

edited

Loading

jyn514 commented Mar 15, 2023

bors commented Mar 31, 2023

calebcartwright commented Apr 22, 2023

estebank Apr 27, 2023

estebank Apr 27, 2023

estebank Apr 27, 2023

estebank Apr 27, 2023

chenyukang Apr 28, 2023 •

edited

Loading

estebank Apr 27, 2023

This comment was marked as outdated.

bors commented May 2, 2023

Dylan-DPC commented May 2, 2023

Dylan-DPC commented May 2, 2023

bors commented May 2, 2023

bors commented May 2, 2023

bors commented May 2, 2023

bors commented May 2, 2023

rust-timer commented May 2, 2023

Noratrieb commented May 2, 2023

pnkfelix commented May 3, 2023

Remove type ascription from parser and diagnostics #109128

Remove type ascription from parser and diagnostics #109128

Conversation

chenyukang commented Mar 14, 2023

rustbot commented Mar 14, 2023

chenyukang commented Mar 14, 2023

rustbot commented Mar 14, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

calebcartwright commented Mar 15, 2023

chenyukang commented Mar 15, 2023

calebcartwright commented Mar 15, 2023

This comment has been minimized.

chenyukang commented Mar 15, 2023 • edited Loading

jyn514 commented Mar 15, 2023

bors commented Mar 31, 2023

calebcartwright commented Apr 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenyukang Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

bors commented May 2, 2023

Dylan-DPC commented May 2, 2023

Dylan-DPC commented May 2, 2023

bors commented May 2, 2023

bors commented May 2, 2023

bors commented May 2, 2023

bors commented May 2, 2023

rust-timer commented May 2, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Noratrieb commented May 2, 2023

pnkfelix commented May 3, 2023

chenyukang commented Mar 15, 2023 •

edited

Loading

chenyukang Apr 28, 2023 •

edited

Loading