Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce proc-macro-srv/sysroot-abi #12835

Merged
merged 18 commits into from
Jul 22, 2022
Merged

Conversation

fasterthanlime
Copy link
Contributor

@fasterthanlime fasterthanlime commented Jul 20, 2022

@fasterthanlime fasterthanlime changed the title Introduce proc-macro-srv/sysroot ABI Introduce proc-macro-srv/sysroot-abi Jul 20, 2022
@fasterthanlime
Copy link
Contributor Author

This now builds (with many stubs).

It can be tested locally with:

cd crates/proc-macro-srv
PROC_MACRO_SRV_SYSROOT_ABI=1 PROC_MACRO_TEST_TOOLCHAIN=stage1 cargo test --features sysroot-abi

Setting PROC_MACRO_TEST_TOOLCHAIN is not necessary if a rust-toolchain.toml is present at the root of rust-analyzer/ with:

[toolchain]
channel = "stage1"

Useful .vscode/settings.json to develop this:

{
    "rust-analyzer.checkOnSave.command": "check",
    "rust-analyzer.cargo.features": [
        "rust-analyzer/in-rust-tree",
        "proc-macro-srv/sysroot-abi"
    ]
}

Otherwise, fall back to the multi ABI scheme, except in testing, where
it becomes a hard error.

This should make it possible to use a rustup-provided rust-analyzer with
proc macro dylibs compiled by older rustcs, and it'll also catch changes
to the format of `rustc --version` or the `.rustc` section that would
make them impossible to compare for equality.
@fasterthanlime
Copy link
Contributor Author

On @bjorn3's suggestion, I added a "full version string exact comparison" when the sysroot ABI is enabled.

In testing, it fails noisily if there's a mismatch - this should only show up in-tree (x.py test) and signals that rustc --version and the .rustc section we read from proc macro dylibs don't match anymore, somehow.

As a binary, if there's a mismatch, it should fall back to numbered ABI implementations gracefully, in case someone has a rustup-provided rust-analyzer binary set as their "RA binary" or "RA proc macro server binary" and expects backwards compatibility.

@fasterthanlime
Copy link
Contributor Author

This PR is now ready for review.

It passes all existing proc-macro-srv tests (including the "literal cloning" test I've added) for the 1.58 ABI, the 1.62 ABI (current stable), and the ABI at https://github.com/rust-lang/rust/commits/a289cfcfb32593c63d75f113547f63ffe2dde285 (I'm going to try again with current master after posting this comment).

When using the "numbered ABIs" (via PROC_MACRO_TEST_TOOLCHAIN), proc-macro-srv tests fail as expected, with a detailed "sysroot mismatch" message:

---- tests::test_attr_macro stdout ----
thread 'tests::test_attr_macro' panicked at 'sysroot ABI mismatch: dylib rustc version (read from .rustc section): "rustc 1.58.1 (db9d1b20b 2022-01-20)" != proc-macro-srv version (read from 'rustc --version'): "rustc 1.64.0-dev"', crates/proc-macro-srv/src/abis/mod.rs:105:21

This will prevent rustc --version / .rustc section mismatches in-tree (y'all should never hit it when developing out-of-tree).

The sysroot-abi is not built in RA's own CI — that would require a pinned nightly and it would be a sync nightmare. It'll only be be built in-tree: the rust-analyzer/in-rust-tree feature enables proc-macro-srv/sysroot-abi.

This doesn't introduce any breaking changes for rust-analyzer users, even if they use the rustup-provided version: the exact version match assertion is only enabled in tests. In production, if there's a sysroot ABI version mismatch, it'll fall back to "numbered ABI" selection, so the situation will at least not be worse (until we teach RA to be smarter about which proc macro srv to use).

The recent changes in proc_macro bridge made some of the "shortcuts" taken by RA more obvious (idents & literals aren't lexed, but they should be). These should be addressed later - the current state of src/abis/abi_sysroot/ra_server.rs is good enough that rust-analyzer should be fully functional.

(For the curious: the only way to notice RA's shortcuts from a proc macro right now is to use the Debug impl of one of proc_macro's public types. I might tackle that in a follow-up PR).

When we merge this, we're good to go for opening another "Re-add rust-analyzer as a subtree" PR to rust-lang/rust.

@fasterthanlime
Copy link
Contributor Author

fasterthanlime commented Jul 21, 2022

⚠️ There's one thing I feel badly re: this PR, and it's the once_cell::sync::Lazy<Mutex<SymbolInterner>>. It's not the Mutex, it's the potential memory leak.

rustc has a concept of "session globals" - their interner is a session global. We don't really have the same concept. I don't know how long-lived instances of ProcMacroProcessSrv are, but if they live for as long as the LSP server itself, memory usage won't go down.

https://github.com/rust-lang/rust/blob/f1a8854f9be2e5cad764d630a53d26c7b72f8162/compiler/rustc_span/src/lib.rs#L82-L91

Ideally we'd use one symbol interner per "expand" call, and that's it. I'm not sure how to make this happen (short of spawning a separate proc macro server for every call) right now.

cc @bjorn3 @Veykril @jonas-schievink


edit: This wasn't needed before because before, any function that could intern a symbol had a &mut self. After the latest proc macro bridge changes, they don't: the server::Server::intern_symbol method is a good example.

Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't review the actual abi implementation, but the version selection logic LGTM modulo a small nit.

crates/proc-macro-srv/src/abis/mod.rs Outdated Show resolved Hide resolved
@fasterthanlime
Copy link
Contributor Author

After chatting with @mystor: all the shortcuts in here are "fine until ra is a subtree" except the Literal formatting, which needs to do something like this:

https://searchfox.org/rust/rev/ceeb5ade201e4181c6d5df2ba96ae5fb2193aadc/library/proc_macro/src/lib.rs#1392-1426

(Even though the ra server only ever builds Literals with a None suffix and a LitKind::Err kind, the proc macro itself could build Literals with an actual suffix/kind and we need to format those properly)

@fasterthanlime fasterthanlime marked this pull request as draft July 21, 2022 15:36
@fasterthanlime fasterthanlime marked this pull request as ready for review July 21, 2022 16:51
crates/proc-macro-srv/src/lib.rs Show resolved Hide resolved
@Veykril
Copy link
Member

Veykril commented Jul 22, 2022

@bors d+

@Veykril
Copy link
Member

Veykril commented Jul 22, 2022

When will I remember...
@bors delegate+

@bors
Copy link
Contributor

bors commented Jul 22, 2022

✌️ @fasterthanlime can now approve this pull request

@fasterthanlime
Copy link
Contributor Author

Nit addressed, :shipit: !

@bors r+

@bors
Copy link
Contributor

bors commented Jul 22, 2022

📌 Commit e591ff3 has been approved by fasterthanlime

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 22, 2022

⌛ Testing commit e591ff3 with merge cb8a3be...

@bors
Copy link
Contributor

bors commented Jul 22, 2022

☀️ Test successful - checks-actions
Approved by: fasterthanlime
Pushing cb8a3be to master...

@bors bors merged commit cb8a3be into rust-lang:master Jul 22, 2022
@lnicola
Copy link
Member

lnicola commented Jul 22, 2022

Thanks a lot for pushing on this, @fasterthanlime!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants