Make fields of `Span` private #43968

petrochenkov · 2017-08-18T00:11:18Z

I actually tried to intern spans and benchmark the result^*, and this was a prerequisite.
This kind of encapsulation will be a prerequisite for any other attempt to compress span's representation, so I decided to submit this change alone.

The issue #43088 seems relevant, but it looks like SpanId won't be able to reuse this interface, unless the tables are global (like interner that I tried) and are not a part of HIR.
r? @michaelwoerister anyway

^* Interning means 2-3 times more space is required for a single span, but duplicates are free. In practice it turned out that duplicates are not that common, so more memory was wasted by interning rather than saved.

petrochenkov · 2017-08-18T00:12:58Z

src/libsyntax_pos/lib.rs

+    }
+    #[inline]
+    #[must_use]
+    pub fn set_lo(self, lo: BytePos) -> Span {


I'm not sure set_x are the best possible names since the functions are not mutating, but at least I guarded them from mistakes with #[must_use].

Should the fields be named with_x? In my mind that conjures the fact that the method returns a copy of the same type with the field having the new value.

Yeah, seems better, I'll rename.

kennytm · 2017-08-18T01:24:00Z

src/librustc/hir/lowering.rs

@@ -425,7 +425,7 @@ impl<'a> LoweringContext<'a> {
                allow_internal_unsafe: false,
            },
        });
-        span.ctxt = SyntaxContext::empty().apply_mark(mark);
+        span = span.set_ctxt(SyntaxContext::empty().apply_mark(mark));
        span


Huh? Just return span.set_ctxt(SyntaxContext::empty().apply_mark(mark)) directly.

estebank · 2017-08-18T06:23:01Z

src/libsyntax_pos/lib.rs

+    #[inline]
+    #[must_use]
+    pub fn set_lo(self, lo: BytePos) -> Span {
+        Span::new(lo, self.hi(), self.ctxt())


Wouldn't it make more sense to do the following in all the set_x methods?

Span {lo, ..self}

It was necessary with interning (the same is true for #43968 (comment)). It will be necessary with any non-trivial Span::new/Span::lo/etc, so I left it.

estebank · 2017-08-18T06:23:06Z

src/libsyntax_pos/lib.rs

+    }
+    #[inline]
+    #[must_use]
+    pub fn set_lo(self, lo: BytePos) -> Span {


Should the fields be named with_x? In my mind that conjures the fact that the method returns a copy of the same type with the field having the new value.

estebank · 2017-08-18T06:24:23Z

src/libsyntax_pos/lib.rs

-        let lo = cmp::max(self.hi.0 - 1, self.lo.0);
-        Span { lo: BytePos(lo), ..self }
+        let lo = cmp::max(self.hi().0 - 1, self.lo().0);
+        self.set_lo(BytePos(lo))


Is it really necessary to use the public getters in the impl?

I guess it would be if spans were really interned. For now I'd also opt for just accessing the fields directly.

michaelwoerister

Looks good to me. Thanks, @petrochenkov! I'm also for renaming the set_x methods to with_x. Other than that (and maybe not using the getters in impl Span) this is good to go.

You might be interested in this: A couple of years ago I did some experiments with span interning plus storing the span information directly in the interning key, if it fit in there (like a tagged pointer). The results were actually quite promising, if I remember correctly: https://internals.rust-lang.org/t/rfc-compiler-refactoring-spans/1357/23. Maybe you want to revisit interning with this additional optimization?

michaelwoerister · 2017-08-18T09:12:34Z

src/libsyntax_pos/lib.rs

-        let lo = cmp::max(self.hi.0 - 1, self.lo.0);
-        Span { lo: BytePos(lo), ..self }
+        let lo = cmp::max(self.hi().0 - 1, self.lo().0);
+        self.set_lo(BytePos(lo))


I guess it would be if spans were really interned. For now I'd also opt for just accessing the fields directly.

petrochenkov · 2017-08-18T10:04:04Z

@michaelwoerister

The results were actually quite promising, if I remember correctly: https://internals.rust-lang.org/t/rfc-compiler-refactoring-spans/1357/23. Maybe you want to revisit interning with this additional optimization?

I remembered there was a thread about this somewhere! Thanks for the link.
Yes, I was going to experiment further with keeping "short" spans inline and other spans "somewhere else".

petrochenkov · 2017-08-18T22:27:23Z

Updated.

estebank · 2017-08-19T03:06:20Z

src/libsyntax/codemap.rs

+            Some(Span::new(
+                cmp::min(sp_lhs.lo(), sp_rhs.lo()),
+                cmp::max(sp_lhs.hi(), sp_rhs.hi()),
+                sp_lhs.ctxt()


I believe this can be replaced with Some(sp_lhs.to(sp_rhs)).

bors · 2017-08-19T05:06:57Z

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

michaelwoerister · 2017-08-19T09:31:28Z

Thanks! r=me after rebasing.

petrochenkov · 2017-08-19T12:37:48Z

@bors r=michaelwoerister

bors · 2017-08-19T12:37:49Z

📌 Commit cdae234 has been approved by michaelwoerister

bors · 2017-08-19T16:28:53Z

⌛ Testing commit cdae2343dba5a47c3e9846b6e7c8521ac1102c2c with merge 17e924eb9a0484958fe948c38b20c93644eb3137...

bors · 2017-08-19T17:34:34Z

💔 Test failed - status-travis

petrochenkov · 2017-08-19T17:39:13Z

Sigh, need to send a PR to rustfmt first.

kennytm · 2017-08-20T08:45:26Z

We've hit a deadlock situation where the rustfmt PR cannot pass without the get/set methods added in this PR, and this PR cannot pass without rustfmt stop reading the now-private fields. I don't think turning rustfmt's status red is a good idea.

I think this PR should be split into two stages,

This PR keep the fields public, so rustfmt doesn't need to be updated at the moment.
rustfmt (as well as clippy, rls, etc.) all accept PRs stop using the fields.
Submit another PR to turn the fields private (can be done simultaneously with step 2).

petrochenkov · 2017-08-20T09:27:52Z

@kennytm
Situations like this were previously resolved by creating a new branch in rustfmt/rls, merging the breaking patch into that branch and not master, then setting rls submodule in rust-lang/rust to that branch.

petrochenkov · 2017-08-20T20:07:13Z

I gathered some span statistics from rustc/libstd and found a significant number of "reverse" spans with lo > hi!
That seems bad, some code may work correctly with such spans, but I've certainly seen ICEs caused by code that doesn't work with them.

I'm going to add "normalization" to lo <= hi into Span::new.
It also boosts the percent of inline spans a bit when compression is used.

to land rust-lang/rust#43968

petrochenkov · 2017-08-20T23:02:47Z

@bors r=michaelwoerister

bors · 2017-08-20T23:02:47Z

📌 Commit 3da163f has been approved by michaelwoerister

bors · 2017-08-20T23:02:57Z

⌛ Testing commit 3da163f55b1dd62f247f63edee30f64e3e7da2f5 with merge 4ad2c420bbe6368916158ddb4b354a7652a02d72...

bors · 2017-08-21T00:50:08Z

💔 Test failed - status-travis

petrochenkov · 2017-08-21T10:07:43Z

@bors r=michaelwoerister

bors · 2017-08-21T10:07:44Z

📌 Commit c4125e2 has been approved by michaelwoerister

bors · 2017-08-21T10:52:17Z

⌛ Testing commit c4125e26389fbe5beceed0c562a47dfb3e45df07 with merge c1ad7551a567319ae597b9643a3dcf627d9bf8e2...

bors · 2017-08-21T12:46:09Z

💔 Test failed - status-travis

kennytm · 2017-08-21T13:02:33Z

x86_64-gnu-aux, same error. It's legit then.

nrc · 2017-08-22T00:41:34Z

RLS tests timed out or some span is not found. Maybe legit

Possibly a bad span produced in save-analysis?

petrochenkov · 2017-08-22T19:13:21Z

The errors are caused by RLS update, not by span patches, i.e. 9061581 alone (without rust-lang/rls@f4e6f16) is enough to reproduce them.
In theory, #44028 should hit them as well.
cc @nrc

estebank · 2017-08-22T21:20:23Z

Could you provide a dump of the reversed spans? I've seen them every now and then but have put off investigating the root cause(s). There are a few possible very unseemly results that could happen, for example when doing end.to(start), so I'd hate to just mask the incorrect spans by doing the obvious fix but rather fix the source (probably during parsing).

petrochenkov · 2017-08-23T21:26:04Z

Waiting on #44028

petrochenkov · 2017-08-23T21:32:16Z

@estebank

Could you provide a dump of the reversed spans?

I have only numbers unfortunately. Looks like the only way to obtain the source of inverted spans from Span::new is backtrace.
Once this PR lands, Span::new will be the single point of span creation, so you will be able to detect inverted spans by inserting an assert(lo <= hi) into it and building everything with RUST_BACKTRACE=1.
EDIT: Maybe that assert could be left there forever once all inverted spans are fixed.

estebank · 2017-08-24T00:43:54Z

Sounds reasonable. Will do. Are asserts only enabled on nightly? Would rather not cause ICEs for a rather benign bug just because it isn't caught in the test, but would like to see he effects of it on cargo bomb.

bors · 2017-08-26T04:09:09Z

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

petrochenkov · 2017-08-26T09:23:10Z

@estebank

Are asserts only enabled on nightly?

Not sure about debug_assert, but assert is always enabled.
FWIW, nightly can be detected through an environmental variable CFG_RELEASE_CHANNEL, but I'm not sure it's a good idea.

to land rust-lang/rust#43968

This helps to avoid landing changes to rustc and rustfmt in one step

petrochenkov · 2017-08-29T22:43:37Z

I'll try to land changes to rustc and rustfmt in two steps (rustfmt is moving forward too fast), so I made fields of Span public again for compatibility.
@bors r=michaelwoerister

bors · 2017-08-29T22:43:37Z

📌 Commit a0c3264 has been approved by michaelwoerister

bors · 2017-08-30T11:08:32Z

⌛ Testing commit a0c3264 with merge ca9cf35...

@michaelwoerister

Make fields of `Span` private I actually tried to intern spans and benchmark the result<sup>*</sup>, and this was a prerequisite. This kind of encapsulation will be a prerequisite for any other attempt to compress span's representation, so I decided to submit this change alone. The issue #43088 seems relevant, but it looks like `SpanId` won't be able to reuse this interface, unless the tables are global (like interner that I tried) and are not a part of HIR. r? @michaelwoerister anyway <sup>*</sup> Interning means 2-3 times more space is required for a single span, but duplicates are free. In practice it turned out that duplicates are not *that* common, so more memory was wasted by interning rather than saved.

bors · 2017-08-30T13:56:51Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing ca9cf35 to master...

rust-highfive assigned michaelwoerister Aug 18, 2017

petrochenkov commented Aug 18, 2017

View reviewed changes

kennytm reviewed Aug 18, 2017

View reviewed changes

estebank reviewed Aug 18, 2017

View reviewed changes

michaelwoerister reviewed Aug 18, 2017

View reviewed changes

petrochenkov force-pushed the span2 branch from 2c1d664 to 9fed6da Compare August 18, 2017 22:26

estebank reviewed Aug 19, 2017

View reviewed changes

mcarton mentioned this pull request Aug 19, 2017

Breakage incoming rust-lang/rust-clippy#1972

Closed

petrochenkov force-pushed the span2 branch from 9fed6da to cdae234 Compare August 19, 2017 12:37

petrochenkov mentioned this pull request Aug 19, 2017

Use getters to access Span fields rust-lang/rustfmt#1899

Closed

nrc added a commit to rust-lang/rls that referenced this pull request Aug 20, 2017

Depend on the span branch of rustfmt

f4e6f16

to land rust-lang/rust#43968

petrochenkov force-pushed the span2 branch from cdae234 to 3da163f Compare August 20, 2017 23:02

nrc mentioned this pull request Aug 22, 2017

Documents managing tools in the Rust repo rust-dev-tools/dev-tools-team#27

Merged

arielb1 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 22, 2017

nrc added a commit to rust-lang/rls that referenced this pull request Aug 27, 2017

Depend on the span branch of rustfmt

903cb9f

to land rust-lang/rust#43968

petrochenkov added 3 commits August 30, 2017 01:38

Make fields of Span private

3da868d

Normalize order of lo and hi in Span::new

71dfe64

Make fields of Span public again

a0c3264

This helps to avoid landing changes to rustc and rustfmt in one step

petrochenkov force-pushed the span2 branch from c4125e2 to a0c3264 Compare August 29, 2017 22:41

bors merged commit a0c3264 into rust-lang:master Aug 30, 2017

This was referenced Aug 30, 2017

Use hir::ItemLocalId instead of ast::NodeId in rustc::middle::region::CodeExtent. #44171

Merged

Add expansion info to crate metadata #43847

Closed

petrochenkov deleted the span2 branch September 10, 2017 22:00

Make fields of Span private #43968

Make fields of Span private #43968

Conversation

petrochenkov commented Aug 18, 2017 • edited Loading

petrochenkov Aug 18, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

michaelwoerister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Aug 18, 2017

petrochenkov commented Aug 18, 2017

Choose a reason for hiding this comment

bors commented Aug 19, 2017

michaelwoerister commented Aug 19, 2017

petrochenkov commented Aug 19, 2017

bors commented Aug 19, 2017

bors commented Aug 19, 2017

bors commented Aug 19, 2017

petrochenkov commented Aug 19, 2017

kennytm commented Aug 20, 2017

petrochenkov commented Aug 20, 2017

petrochenkov commented Aug 20, 2017

petrochenkov commented Aug 20, 2017

bors commented Aug 20, 2017

bors commented Aug 20, 2017

bors commented Aug 21, 2017

petrochenkov commented Aug 21, 2017

bors commented Aug 21, 2017

bors commented Aug 21, 2017

bors commented Aug 21, 2017

kennytm commented Aug 21, 2017

nrc commented Aug 22, 2017

petrochenkov commented Aug 22, 2017 • edited Loading

estebank commented Aug 22, 2017

petrochenkov commented Aug 23, 2017

petrochenkov commented Aug 23, 2017 • edited Loading

estebank commented Aug 24, 2017

bors commented Aug 26, 2017

petrochenkov commented Aug 26, 2017 • edited Loading

petrochenkov commented Aug 29, 2017

bors commented Aug 29, 2017

bors commented Aug 30, 2017

bors commented Aug 30, 2017

Make fields of `Span` private #43968

Make fields of `Span` private #43968

petrochenkov commented Aug 18, 2017 •

edited

Loading

petrochenkov Aug 18, 2017 •

edited

Loading

petrochenkov commented Aug 22, 2017 •

edited

Loading

petrochenkov commented Aug 23, 2017 •

edited

Loading

petrochenkov commented Aug 26, 2017 •

edited

Loading