Stop sorting `Span`s' `SyntaxContext`, as that is incompatible with incremental by oli-obk · Pull Request #123165 · rust-lang/rust

oli-obk · 2024-03-28T10:55:12Z

work towards #90317

Luckily no one actually needed these to be sorted, so it didn't even affect diagnostics. I'm guessing they'd have been sorted by creation time anyway, so it wouldn't really have mattered.

r? @cjgillot

cjgillot · 2024-03-30T00:25:58Z

            lo: s_lo,
            hi: s_hi,
-            ctxt: s_ctxt,
+            ctxt: _,


This needs a comment for macro expansions.
Side note: this Ord impl is not consistent with the derived impl for PartialEq. Should we fix the latter?

What do you think about ensuring that for equal (including the sort-ignored fields) SpanDatas we just return Ordering::Equal, and only sort the others?

That's not consistent either.
The a == b implies partial_cmp(a, b) == Some(Equal) direction is easy. We have it without adding a special case.
The converse is harder: if s_lo == o_lo && s_hi == o_hi we get Some(Equal), but s_ctxt may not be o_ctxt.

~~Maybe Spans should just be PartialOrd, but not Ord. I'll try if we can get away with just removing Ord~~

Edit: ah no, we need Ord for sort and friends.

Yea, idk how to solve this in a way that makes everything work out, except go full in with https://github.com/rust-lang/rust/compare/master...oli-obk:rust:no_ord_span?expand=1, no matter how annoying it is

petrochenkov · 2024-04-03T11:45:07Z

Is it hard to remove PartialOrd from Span and use some explicit location_only_ordering_hygiene_is_ignored method instead?
If the change affects SyntaxContext only, then the cure seems to be worse than the disease.

oli-obk · 2024-04-03T13:49:33Z

@rustbot author

I'll explore that, but I think at least borrowck uses it to sort its diagnostics

oli-obk · 2024-04-19T13:46:38Z

Is it hard to remove PartialOrd from Span and use some explicit location_only_ordering_hygiene_is_ignored method instead? If the change affects SyntaxContext only, then the cure seems to be worse than the disease.

I tried it, but sorting spans is used all over the compiler, even if we added convenience helpers for it, the right way would be to replace most of these spans with a SpanSortedByBytePos wrapper struct, everything else is too annoying. See https://github.com/rust-lang/rust/compare/master...oli-obk:rust:no_ord_span?expand=1 for a hacky way towards that

…ncremental

rustbot · 2024-04-19T13:55:18Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

cjgillot · 2024-06-21T13:28:06Z

@bors r+ rollup=never

bors · 2024-06-21T13:28:08Z

📌 Commit 4239a73 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2024-06-21T13:47:47Z

⌛ Testing commit 4239a73 with merge 25c9f2c...

bors · 2024-06-21T15:59:06Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 25c9f2c to master...

rust-timer · 2024-06-21T17:15:20Z

Finished benchmarking commit (25c9f2c): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

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.3%, 0.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.3%, 0.3%]	1

Max RSS (memory usage)

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

Cycles

Results (secondary -2.6%)

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
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	2
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 694.36s -> 694.089s (-0.04%)
Artifact size: 326.84 MiB -> 326.85 MiB (0.00%)

`Span` and `SpanData` encode four fields: `lo`/`hi`, `ctxt`, and `parent`. `ctxt` and `parent` have unorderable types. Both types ignore `ctxt` and `parent` for `PartialOrd`/`Ord`. But their `PartialEq`/`Eq` impls do *not* ignore those fields. This eq/ord inconsistency is a bug. It was introduced in rust-lang#123165. The idea of ordering spans in general is dubious, because of `ctxt` and `parent`. But the idea of ordering spans just with `lo`/`hi` is fine. Therefore, this commit does the following. - Removes the `PartialOrd`/`Ord` impls for `Span`/`SpanData` - Adds a `Span::lo_hi` method which can be used in lots of places where span locations are involved in sorting. E.g. `xs.sort_by_key(|span| span.lo_hi())` - Adds `OrdSpan`, a newtype around `Span` that impls `PartialEq`/`Eq`/`PartialOrd`/`Ord` using `lo_hi`. This is for storing spans in ordered types like `BTreeMap<OrdSpan, T>`. Note also that some `sort`+`dedup` combinations might not remove all duplicates with the old eq/ord inconsistency. These now all do the right thing, which could affect some error messages, though in practice nothing in the test suite is affected.

rustbot assigned cjgillot Mar 28, 2024

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 28, 2024

cjgillot reviewed Mar 30, 2024

View reviewed changes

cjgillot 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 30, 2024

oli-obk force-pushed the no_ord_def_id3 branch from 7bb758b to dfe4666 Compare April 2, 2024 13:42

oli-obk added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 2, 2024

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2024

Stop sorting Spans' SyntaxContext, as that is incompatible with i…

4239a73

…ncremental

oli-obk force-pushed the no_ord_def_id3 branch from dfe4666 to 4239a73 Compare April 19, 2024 13:55

oli-obk added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 3, 2024

oli-obk mentioned this pull request Jun 4, 2024

coverage: Carve out hole spans in a separate early pass #125921

Merged

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 Jun 21, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 21, 2024

bors merged commit 25c9f2c into rust-lang:master Jun 21, 2024

rustbot added this to the 1.81.0 milestone Jun 21, 2024

oli-obk deleted the no_ord_def_id3 branch January 20, 2025 12:09

Navigation Menu

Repository navigation

Stop sorting `Span`s' `SyntaxContext`, as that is incompatible with incremental - #123165

Stop sorting `Span`s' `SyntaxContext`, as that is incompatible with incremental#123165
bors merged 1 commit into
rust-lang:masterfrom
oli-obk:no_ord_def_id3

oli-obk commented Mar 28, 2024

Uh oh!

cjgillot Mar 30, 2024

Uh oh!

oli-obk Apr 2, 2024

Uh oh!

cjgillot Apr 2, 2024

Uh oh!

oli-obk Apr 19, 2024 •

edited

Loading

Uh oh!

petrochenkov commented Apr 3, 2024

Uh oh!

oli-obk commented Apr 3, 2024

Uh oh!

oli-obk commented Apr 19, 2024

Uh oh!

rustbot commented Apr 19, 2024

Uh oh!

cjgillot commented Jun 21, 2024

Uh oh!

bors commented Jun 21, 2024

Uh oh!

bors commented Jun 21, 2024

Uh oh!

bors commented Jun 21, 2024

Uh oh!

rust-timer commented Jun 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

oli-obk commented Mar 28, 2024

Uh oh!

cjgillot Mar 30, 2024

Choose a reason for hiding this comment

Uh oh!

oli-obk Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

cjgillot Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

oli-obk Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Apr 3, 2024

Uh oh!

oli-obk commented Apr 3, 2024

Uh oh!

oli-obk commented Apr 19, 2024

Uh oh!

rustbot commented Apr 19, 2024

Uh oh!

cjgillot commented Jun 21, 2024

Uh oh!

bors commented Jun 21, 2024

Uh oh!

bors commented Jun 21, 2024

Uh oh!

bors commented Jun 21, 2024

Uh oh!

rust-timer commented Jun 21, 2024

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

oli-obk Apr 19, 2024 •

edited

Loading