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

internal: Teach cargo about cfg(rust_analyzer) #16257

Merged
merged 1 commit into from Apr 18, 2024

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Jan 4, 2024

r? @Urgau is this a good idea?, CC @Veykril

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 4, 2024
@Veykril
Copy link
Member

Veykril commented Jan 4, 2024

Is this so we can remove the cfg from bootstrap again?

@lnicola
Copy link
Member Author

lnicola commented Jan 4, 2024

Maybe, but I'm not too keen on trying it right now.

See https://blog.rust-lang.org/inside-rust/2024/01/03/this-development-cycle-in-cargo-1-76.html#-zcheck-cfg, I don't know if it will be enabled by default in the future.

@Veykril
Copy link
Member

Veykril commented Jan 4, 2024

I see, well fine with me either way.

@lnicola
Copy link
Member Author

lnicola commented Jan 4, 2024

warning: proc-macro-srv@0.0.0: cargo:rustc-check-cfg requires -Zcheck-cfg flag

This is a bit annoying, I guess it will have to wait.

@lnicola lnicola marked this pull request as draft January 4, 2024 16:40
@Urgau
Copy link

Urgau commented Jan 4, 2024

This looks alright, but as noted it won't work if cargo -Zcheck-cfg isn't used.

See https://blog.rust-lang.org/inside-rust/2024/01/03/this-development-cycle-in-cargo-1-76.html#-zcheck-cfg, I don't know if it will be enabled by default in the future.

The goal is to have it enable by default (and therefore remove -Zcheck-cfg from cargo). We still still have somethings to iron out before stabilization can happen but I'm hoping it will be stabilized and enabled by default before mid-2024.

@lnicola lnicola added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2024
@Veykril
Copy link
Member

Veykril commented Apr 18, 2024

Blocked on rust-lang/cargo#13571

@Urgau
Copy link

Urgau commented Apr 18, 2024

Blocked on rust-lang/cargo#13571

Note that since rust-lang/cargo#13438, Cargo no longer emits a warning when using cargo::rustc-check-cfg in stable/beta (>=1.77). The instruction will just be silently ignored.

@lnicola lnicola marked this pull request as ready for review April 18, 2024 11:56
@Veykril
Copy link
Member

Veykril commented Apr 18, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 18, 2024

📌 Commit 3a2ae64 has been approved by Veykril

It is now in the queue for this repository.

@@ -0,0 +1,5 @@
//! This teaches cargo about our cfg(rust_analyzer)
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why I added these, but.. 😅.

@bors
Copy link
Collaborator

bors commented Apr 18, 2024

⌛ Testing commit 3a2ae64 with merge af1fd88...

@bors
Copy link
Collaborator

bors commented Apr 18, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing af1fd88 to master...

@bors bors merged commit af1fd88 into rust-lang:master Apr 18, 2024
10 checks passed
@lnicola lnicola deleted the rustc-check-cfg branch April 18, 2024 12:27
@epage
Copy link

epage commented Apr 18, 2024

To double check, is cfg(rust_analyzer) something used exclusively within rust-analyzer for its own development or does rust-analyzer always set it when it runs so code can detect when its being built by rust-analyzer to do things differently?

If its the latter, that sounds like a "well known cfg" and should likely be defined in rustc along with miri and others or cargo along with docsrs.

@Veykril
Copy link
Member

Veykril commented Apr 18, 2024

rust-analyzer sets this for workspace crates of a loaded project when doing its own analysis, but not when invoking cargo or any other tool. It's strictly contained to its analysis only.

@Urgau
Copy link

Urgau commented Apr 18, 2024

I was under the impression that the rust_analyzer cfg was a internal cfg (due to being used for internal things), but now looking at the code I'm no longer certain, it seems to be unconditionally added to every Cargo project:

// Add test cfg for local crates
if cargo[pkg].is_local {
cfg_options.insert_atom("test".into());
cfg_options.insert_atom("rust_analyzer".into());
}

and also every "detached" Rust files
cfg_options.insert_atom("rust_analyzer".into());

@Veykril Is my analysis right, that the rust_analyzer cfg is added to every compilation unit by rust analyzer? If yes, is that cfg considered for public use (in the sense that users can use it)?

EDIT: raced with @Veykril 😄

@Veykril
Copy link
Member

Veykril commented Apr 18, 2024

Did that answer your question already? 😄 Effectively to cargo, the cfg will always be disabled (unless the user somehow passes it to cargo themselves), the cfgs linked in the code there are the ones that r-a sees, but not any invoked tooling.

@Urgau
Copy link

Urgau commented Apr 18, 2024

Did that answer your question already?

I think so. From what I understand during rust-analyzer analysis of the code r-a adds (an analysis-only) rust_analyzer cfg, which is never given to any tooling.

And then there is a second rust_analyzer cfg, the one used for the proc-macro-srv crates, which is "real" but for internal use only.

If my understanding is right, then every is good, rust_analyzer is not a well-known cfg.

@Veykril
Copy link
Member

Veykril commented Apr 18, 2024

It is not well known as it's fairly recent, and mainly an escape hatch to be able to cfg out things that are very slow for the ide (like the quote crate).

And then there is a second rust_analyzer cfg, the one used for the proc-macro-srv crates, which is "real" but for internal use only.

There it's also only used by analysis, its just that those crates are the only ones in the r-a codebase that actually have a cfg(rust-analyzer) in it.

@Urgau
Copy link

Urgau commented Apr 18, 2024

It is not well known as it's fairly recent

My bad, I was referring to well-known in the sense of --check-cfg well known names and values.

There it's also only used by analysis, its just that those crates are the only ones in the r-a codebase that actually have a cfg(rust-analyzer) in it.

This seems very similar to the miri and clippy cfgs, which are used to prevent the tool from going somewhere they don't handle (well).

So users can expect the rust_analyzer cfg to be present and set by r-a?

@Veykril
Copy link
Member

Veykril commented Apr 18, 2024

So users can expect the rust_analyzer cfg to be present and set by r-a?

Yes but only on workspace member crates (so not dependencies in the crates-io registry), to prevent libraries from special casing rust-analyzer for the time being.

@Urgau
Copy link

Urgau commented Apr 18, 2024

If you don't want users to "special case rust-analyzer for the time being", does that also mean that you don't want to advertise the cfg ? and if so, are fine with users having to expect the cfg in a build.rs ?

More broadly the question is: should rustc know (and by extension advertise) the rust_analyzer cfg ?

@Veykril
Copy link
Member

Veykril commented Apr 18, 2024

Having it be known is fine I think, it's not going away I'd say. The restriction is only there to deter people from doing weird r-a specific things in their libraries for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants