Make proc_macro::Group take up less space remove redundant field.

[Keith-Cancel]

The field entire is redundant. This saves 4 bytes and makes the proc_macro::DelimSpan match up with what the compiler uses. Moreover Group::set_span() overwrites all the fields anyways so there not is any chance for entire not to be derived from the open and close. Lastly, the way the compiler constructs the entire field is also be based off the open and close.

This takes the size from 20 bytes to 16 bytes which is also a nice power of 2.

[rustbot]

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

[Keith-Cancel]

Okay, it looks like for whatever reason pr-check-1 expects entire not to be there, but pr-check-2 expects entire to be there. They must not be using the same source??

-_-

locally I am able to execute ./x test without any issues

[clarfonthey]

There is actually a script to run the CI jobs locally with Docker: https://rustc-dev-guide.rust-lang.org/tests/docker.html#testing-with-docker

From what it seems at first glance, the tests are just running ./x check, but there might be some extra config being added that makes the tests run which isn't the default.

[Keith-Cancel]

There is actually a script to run the CI jobs locally with Docker: https://rustc-dev-guide.rust-lang.org/tests/docker.html#testing-with-docker

From what it seems at first glance, the tests are just running ./x check, but there might be some extra config being added that makes the tests run which isn't the default.

I wondering if it's because rust analyzer is a separate repo? Is pr-check-2 pulling the source from the repo?

[clarfonthey]

rust-analyzer is a subtree intentionally to avoid issues like this: you can edit the code in this repo if it needs to be fixed to get both to compile, and then at the next regular merge someone will make sure the repo and the subtree match.

[Keith-Cancel]

rust-analyzer is a subtree intentionally to avoid issues like this: you can edit the code in this repo if it needs to be fixed to get both to compile, and then at the next regular merge someone will make sure the repo and the subtree match.

Which is what I did, I am just not how or why one check is using the older version not in the PR. I tried looking at the CI directory for those briefly nothing immediately sprung out at me.

[clarfonthey]

Yeah, that's mostly why I recommended trying the docker approach because it will give you a better idea of what's running and thus what's failing. Since you can investigate the containers yourself, it'll tell you what's wrong.

[Veykril]

That is weird ... rust-analyzer should be building against the intro library sources ...

[Veykril]

cc @jieyouxu any idea what is going on here? It seems like rust-analyzer is not being built against the in-tree library when tested?

[ChayimFriedman2]

Didn't we remove the in-tree dependency, because T-types asked us to not depend on the in-tree trait solver?

[ShoyuVanilla]

Didn't we remove the in-tree dependency, because T-types asked us to not depend on the in-tree trait solver?

I guess this isn't the case for proc_macro

[bjorn3]

Yeah, for proc-macro rust-analyzer has to use the in-tree version for the rust-analyzer-proc-macro-server shipped with rustc to be ABI compatible with proc-macros built by said rustc.

[Veykril]

Right. Yea we definitely do want to built against the in-tree library still.

[petrochenkov]

Why does it make sense to optimize for the size of Group rather than for the speed of Group::entire?
This is not an obvious choice.

(In any case, CI is failing.)
@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Keith-Cancel]

@petrochenkov

Why does it make sense to optimize for the size of Group rather than for the speed of Group::entire? This is not an obvious choice.

Well when using the proc_macro API I find my self storing the Group Tokens when parsing more often than getting the span. Especially, since there is not a way to reconstruct a Group token without loss of information with the way set_span() works. However, the span is normally only used for diagnostics in a proc_macro in my experience which is then either emitted via a compile_error!() in stable or in nightly via the diagnostics API. I would consider a emitting diagnostics a cold path. That's also part of my reasoning why it seems prudent to optimize for memory.

(In any case, CI is failing.) @rustbot author

That is something odd going on, 2 of the CI tests are using different source files. It even got the eyes of some other people.

Some Tangential Thoughts

Related to the join function having to a bridge call and potential performance. There is also this issue #130856. I feel instead of span being opaque handle. It might make sense to have proc_macro::Span have a representation closer to what the compiler uses. That would make manipulation faster without bridge. This also allows it to have meaning and use in instances without rustc. That is a separate issue from this, but more of just a thought. However, this would make the span manipulation functions as cheap in proc_macro like they are in the rustc compiler which generally are quite cheap. Although, the number of manipulation functions exposed are quite limited even in nightly in the proc_macro library.

The old span representation in the compiler was 4 bytes from what I understand. The code fragments that proc_macros process are generally smaller. Therefore something along those lines might be acceptable, and still keep the proc_macro span as 4 bytes.

[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.

[petrochenkov]

Okay, the motivation looks convincing enough.
r=me after squashing commits.
@rustbot author

[Keith-Cancel]

Okay I squashed the commits to a single commit.

r? @petrochenkov

@rustbot ready

[rustbot]

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

[petrochenkov]

@bors r+

[bors]

📌 Commit 8b82c4c has been approved by petrochenkov

It is now in the queue for this repository.

[petrochenkov]

@bors r-
@rusbot blocked on some decisions in #149052 (comment).