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

Fix imports of built-in macros (mostly) #61877

Closed
wants to merge 3 commits into from

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jun 15, 2019

Built-in macros now have DefIds from LOCAL_CRATE and their DefIndexes occupy a continuous fixed-size range immediately following GlobalMetaData entries.

This way they can be used in imports without causing ICEs due to CrateNum::BuiltinMacros.
I tried a few different approaches mentioned in #61687, but this one seems to be the least ugly and painful.
EDIT: One more alternative is to follow the example of Res::PrimTy(hir::PrimTy) and introduce Res::BuiltinMacro(usize) so that built-in macros don't need a DefId at all. I didn't try it yet, but perhaps I should.

Caveats:

  • Same built-in macro imported through different crates has different DefIds.
    This can be observable in corner cases (e.g. glob vs glob conflicts, see the test import-builtin-macros-bug.rs ).
    I tried to convert (THAT_CRATE, DefIndex) into (LOCAL_CRATE, DefIndex) for built-in macros immediately during decoding, but apparently there are assumptions that entries from other crates cannot have DefId from the current crate after decoding, so it caused ICEs.
  • Individual imports of built-in macros are not stability checked (e.g. use concat_idents; does not produce a feature error) so the whole feature is gated instead.
    The primary motivation for the fix is reexporting built-in macros from the standard library and we can use unstable features in the standard library.
    The proper solution to this (and multiple other) problems is moving stability/deprecation checking into resolve.

Closes #61687
cc #61875
r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2019
@@ -570,6 +570,9 @@ declare_features! (
// #[repr(transparent)] on unions.
(active, transparent_unions, "1.37.0", Some(60405), None),

// `use builtin_macro`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// `use builtin_macro`.
// Allows `use builtin_macro;`.

@@ -0,0 +1,10 @@
// FIXME: Individual imports of built-in macros are not stability checked right now,
// so the whole feature is gated instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this comment means. Can you illustrate the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use concat_idents as unstable; below should produce a "concat_idents is unstable" error, but it doesn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see; I read .stderr sloppily... thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahaha, concat_idents and friends are actually stable themselves, they just report feature errors as a part of their expansion.
I'll fix this, but #61898 is a pre-requisite.

@petrochenkov
Copy link
Contributor Author

I'm going to try the Res::BuiltinMacro approach, marking as waiting-on-author.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2019
@eddyb
Copy link
Member

eddyb commented Jun 16, 2019

Something I really wanted years ago was to move the "definitions" of builtin types into libcore.
That is, to have #[intrinsic = "u8"] type u8; or #[intrinsic = "u8"] enum u8 {} or w/e somewhere in libcore, and have that be the canonical definition, to get rid of some of the hardcoding.
(I guess nowadays you could also abuse extern "intrinsic" { type u8; })

So I'm wondering, can we put #[intrinsic = "concat_idents"] macro concat_idents; in libcore?
Not a reexport of some magical item, but a definition with implementation-specified semantics.
Even if we haven't done it for builtin types yet, maybe we can do it for macros to start with?

@petrochenkov
Copy link
Contributor Author

So I'm wondering, can we put #[intrinsic = "concat_idents"] macro concat_idents; in libcore?

That's an interesting idea.
Attaching all this data associated with built-in macros (like stability) to an actual node in source code would make many things simpler.
I'll try it.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jun 16, 2019

For built-in types it was tried in #32274.
(The downside is some potential inconveniences for libcore itself and other code using no_core or no_imlicit_prelude, but that depends on how exactly this is implemented.)

For built-in macros it may work generally better because they are not so fundamentally tied to the compiler, the compiler just provides expander functions for them, otherwise they are usual macros.

@petrochenkov
Copy link
Contributor Author

Built-in macros introduced through libcore are blocked on #61898.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 16, 2019
@nikomatsakis
Copy link
Contributor

@eddyb

Nit: I feel like these should be lang items -- I'm not sure why we have so many categories of "special thing known to the compiler" but I think we should centralize on one of them.

@eddyb
Copy link
Member

eddyb commented Jun 18, 2019

@nikomatsakis IMO we should distinguish between "this is exposing a compiler primitive to the library" and "this is exposing a library entity to the compiler", only the latter of which needs to be unique.

Centril added a commit to Centril/rust that referenced this pull request Jun 18, 2019
syntax: Factor out common fields from `SyntaxExtension` variants

And some other related cleanups.

Continuation of rust-lang#61606.
This will also help to unblock rust-lang#61877.
Centril added a commit to Centril/rust that referenced this pull request Jun 18, 2019
syntax: Factor out common fields from `SyntaxExtension` variants

And some other related cleanups.

Continuation of rust-lang#61606.
This will also help to unblock rust-lang#61877.
@bors
Copy link
Contributor

bors commented Jun 19, 2019

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

@matklad
Copy link
Member

matklad commented Jun 19, 2019

Something I really wanted years ago was to move the "definitions" of builtin types into libcore.

Yess, I have been thinking exactly about this when adding primitives names resolution to rust-analyzer. It also helps with goto definition UX in IDEs, because you have the actual source to go to.

It probably shouldn't be literally libcore though, because no_core is a thing. Rather we need a separate builtins crate which you can't opt out of.

@mark-i-m
Copy link
Member

It probably shouldn't be literally libcore though, because no_core is a thing. Rather we need a separate builtins crate which you can't opt out of.

libreallycore

@eddyb
Copy link
Member

eddyb commented Jun 19, 2019

It probably shouldn't be literally libcore though, because no_core is a thing. Rather we need a separate builtins crate which you can't opt out of.

Nope, #![no_core] exists literally for libcore alone. It's really hard to have any other setup, because a bunch of lang items would have to be in it for anything to work, not just the builtin types/macros.

EDIT: worst case, this will change and we can reexport those items in libcore backwards-compatibly.

@petrochenkov
Copy link
Contributor Author

Now waiting on #62042

@petrochenkov
Copy link
Contributor Author

#62086 confirmed that the idea with defining built-in macros through libcore is viable, so I'll close this PR.

@petrochenkov petrochenkov deleted the usbumac3 branch July 1, 2019 22:56
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.

Imports/reexports of built-in macros are not supported
8 participants