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

Enable proc macros by default #7328

Closed
matklad opened this issue Jan 18, 2021 · 11 comments
Closed

Enable proc macros by default #7328

matklad opened this issue Jan 18, 2021 · 11 comments
Labels
S-actionable Someone could pick this issue up and work on it right now

Comments

@matklad
Copy link
Member

matklad commented Jan 18, 2021

Proc macros are opt-in, we should make them opt-out. Part of #7325 (comment)

Steps:

  • cleanup settings naming, docs, location and semantics. Current settings are a bit of a mess.
  • make sure that proc macros are loaded asynchronously. At the moment, we run cargo check and cargo metadata together. Instead, we should first run only metadata, immediatelly start suggesting cod insight, and load cargo check info in backgrooud. This should also pave the way for reloading things when build.rs / proc macros change.
  • (ideally) implement ABI detection, so that we don't silently crash on ABI misatch
@matklad matklad mentioned this issue Jan 18, 2021
4 tasks
@matklad
Copy link
Member Author

matklad commented Jan 18, 2021

cc #6448 and @edwin0cheng

@edwin0cheng
Copy link
Member

(ideally) implement ABI detection, so that we don't silently crash on ABI misatch

#6822 maybe the first step

@edwin0cheng
Copy link
Member

make sure that proc macros are loaded asynchronously. At the moment, we run cargo check and cargo metadata together. Instead, we should first run only metadata, immediatelly start suggesting cod insight, and load cargo check info in backgrooud. This should also pave the way for reloading things when build.rs / proc macros change.

Do we want to integrate it with flycheck ?

@matklad
Copy link
Member Author

matklad commented Jan 19, 2021

I don't think so, they have different conditions for when they must be recomputed.

@edwin0cheng
Copy link
Member

One of the problems here is cargo check will block another cargo check, and it will make users confused what happen under the hood

We have to make sure the progress bar will provide enough information about it.

@edwin0cheng
Copy link
Member

@matklad , after #7387, BuildData represents the build script related data we fetch from cargo check.

And next step will be putting it into salsa as input (with a key of CrateId).

@matklad
Copy link
Member Author

matklad commented Jan 22, 2021

And next step will be putting it into salsa as input (with a key of CrateId).

Hm, I think we just need to set the crate graph, rather than introducing a new input? We'll need to modify the graph for features anyway. I imagine we want to add build_info: Option<BuildInfo> to to_crate_graph function. Then, the global state will hold current metadata result, current build info, and merge them as appropriate.

@edwin0cheng
Copy link
Member

edwin0cheng commented Jan 22, 2021

Just want to clarify :

If I understand correctly, In your approach, when I change a single build.rs in dependency, a modifed CrateGraph will be set to salsa. But almost all things depends on CrateGraph in salsa, so they will be have to rebuild, right ? Did I miss something here?

@matklad
Copy link
Member Author

matklad commented Jan 22, 2021

If I understand correctly, In your approach, when I change a single build.rs in dependency, a modifed CrateGraph will be set to salsa. But almost all things depends on CrateGraph in salsa, so they will be have to rebuild, right

Right! The same already happens to Cargo.toml edit. I think that long term we should be smarter here and don't store crate graph as a single value in salsa, but this is orthogonal to the work here.

@lnicola lnicola added the S-actionable Someone could pick this issue up and work on it right now label Jan 22, 2021
bors bot added a commit that referenced this issue Jan 28, 2021
7412: Async loading for outdir and proc-macro r=maklad a=edwin0cheng

cc #7328

![Peek 2021-01-24 02-04](https://user-images.githubusercontent.com/11014119/105610083-8f208100-5de8-11eb-8e96-c2d4e349b352.gif)

[Edit] 
~~Finding a way to know when the workspace and build data are loaded...~~

[Edit 2] 
Not perfect solution, but seem to work now.


Co-authored-by: Edwin Cheng <[email protected]>
@edwin0cheng
Copy link
Member

implement ABI detection, so that we don't silently crash on ABI mismatches

Some crazy ideas:

Can we use the debug information ot figure out the ABI in debug=1 case (and at least in CI) ? What I mean is, we could extract the dwarf information from a dynamic library and find out some important functions & structures signatures in src/proc_macro/bridge/client.rs. @bjorn3 do you have any experience about it ?

Another idea is, we could use rustc-nightly in CI and compare these structures side by side, but it will only detect what changed in nightly but not stable.

@bjorn3
Copy link
Member

bjorn3 commented Feb 8, 2021

The DWARF emitted by rustc is unstable. You need debug=2 by the way for figuring out the ABI. debug=1 only enables lineinfo. It seems that proc_macro is only compiled with debug=1, so the necessary info is missing:

$ dwarfdump -i $(rustc --print sysroot)/lib/rustlib/x86_64-unknown-linux-gnu/lib/libproc_macro-7a14b765860d45a7.rlib | grep DW_TAG_ | grep -v DW_TAG_inlined_subroutine | grep -v DW_TAG_subprogram | grep -v DW_TAG_namespace
< 0><0x0000000b>  DW_TAG_compile_unit
Can't process archive member 1 lib.rmeta of ~/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libproc_macro-7a14b765860d45a7.rlib: unknown format

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

No branches or pull requests

4 participants