view-types: store view types in the AST

[scrabsha]

View all comments

Tracking issue: #155938.

Instead of discarding view types, we store them in the AST now!

[JonathanBrouwer]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 9d3dea7 (9d3dea74651f758e35177070e510be8f3639a7fd, parent: 6769f690f947f12e36db6b6503bab265b7b2cced)

[rust-timer]

Finished benchmarking commit (9d3dea7): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-1.4%, -0.3%]	6
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.1%]	2
All ❌✅ (primary)	-0.7%	[-1.4%, -0.3%]	6

Max RSS (memory usage)

Results (primary 0.1%, secondary -2.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.9%	[1.9%, 1.9%]	1
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	1
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.7%]	2
Improvements ✅ (secondary)	-4.4%	[-7.3%, -1.5%]	2
All ❌✅ (primary)	0.1%	[-0.8%, 1.9%]	3

Cycles

Results (secondary 3.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.0%	[0.5%, 9.9%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.3%	[-1.3%, -1.3%]	1
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 503.61s -> 496.571s (-1.40%)
Artifact size: 394.46 MiB -> 394.47 MiB (0.00%)

[scrabsha]

^

• rebased to main to fix conflicts
• fixed view-types: store view types in the AST #156016 (comment)

r? nikomatsakis

[scrabsha]

@rustbot reroll

edit: did y'all know there was a shortcut for "comment and close"?

[rustbot]

Error: Assignment is not allowed on a closed PR.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

[scrabsha]

@rustbot reroll

[BoxyUwU]

r? compiler

[BoxyUwU]

ah apparently oli was talked to privately about this,

r? oli-obk

[oli-obk]

for pattern types I used a builtin macro instead of starting out with syntax. While the reason for that was that it was a breaking change due to the follow-rules of ty fragments allowing is after it. Is this not an issue for .?

Further advantages were that it's syntax that is "automatically" supported in r-a and syn, and is much easier to edit and fine tune

View changes since the review

[scrabsha]

for pattern types I used a builtin macro instead of starting out with syntax. While the reason for that was that it was a breaking change due to the follow-rules of ty fragments allowing is after it. Is this not an issue for .?

Oh, nice catch. I'll switch to pattern_types!(Foo.{ fields }) immediately. Thanks for mentioning this!f

(For those at home who are trying to figure out how this can break code, #107606 (comment) is a good repro)

Further advantages were that it's syntax that is "automatically" supported in r-a and syn, and is much easier to edit and fine tune

Agreed. r-a screams at me every time i open my tests hehe

[scrabsha]

(writing this here so i avoid the pain of having to discover this again)

the same problem arises for self.{ fields } as well, for instance with this macro:

macro_rules! foo {
    ($it:item) => {};
    (impl Foo { fn bar(self.{ meow }) {} }) => {};
}

foo!(impl Foo { fn bar(self.{ meow }) {} });

we can't have nice things :(

[fmease]

I don't see how your example is problematic, it already doesn't compile, so it won't be a regression. However, it's true that the addition of the view types syntax did regress code.

E.g., the following snippet compiles on stable but it no longer compiles on beta & nightly:

macro_rules! m {
    ($ty:ty) => { /*compile_error!("");*/ };
    (&().{}) => {};
}

m!(&().{});

[scrabsha]

I don't see how your example is problematic, it already doesn't compile, so it won't be a regression.

is this another instance of #103534?

(ack for the rest of your message, i will fix it soon)

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[scrabsha]

^

• rebased over main
• added the view_type builtin macro replace the $ty.{ field } syntax with view_type!($ty.{ field }) (i took a lot of inspiration from Implement minimal, internal-only pattern types in the type system #120131 :3)
• added a test for the regression reported in view-types: store view types in the AST #156016 (comment)
• removed the self.{ field } syntax. i will bring this back in a later PR

@rustbot ready

[scrabsha]

this file has been moved to tests/ui/view-types/feature-gate-view-types.rs (which is, AIUI what's done for pattern types)

View changes since the review

[rust-log-analyzer]

The job pr-check-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

---- library/core/src/view.rs - view::view_type (line 4) stdout ----
error: expected identifier, found `=`
  --> library/core/src/view.rs:11:6
   |
11 | type = std::view::view_type!(Foo.{ bar });
   |      ^ expected identifier

error: aborting due to 1 previous error

Couldn't compile the test.
\ (no newline at end of output)
---- library/core/src/view.rs - view::view_type (line 4) stdout end ----

failures:
    library/core/src/view.rs - view::view_type (line 4)

test result: FAILED. 1414 passed; 1 failed; 5 ignored; 0 measured; 0 filtered out; finished in 56.24s