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

Rollup of 16 pull requests #73950

Merged
merged 41 commits into from
Jul 2, 2020
Merged

Rollup of 16 pull requests #73950

merged 41 commits into from
Jul 2, 2020

Conversation

Manishearth
Copy link
Member

Successful merges:

Failed merges:

r? @ghost

ChrisDenton and others added 30 commits May 25, 2020 13:53
On Windows the InnoSetup installer was superseded by the MSI installer. It's no longer needed.
This commit applies the existing 'extra angle bracket recovery' logic
when parsing fields in struct definitions. This allows us to continue
parsing the struct's fields, avoiding spurious 'missing field' errors in
code that tries to use the struct.
Use back-ticks instead of quotation marks in docs for the block comment
variant of TokenKind.
There were a few instances of this pattern:

```rust
while index < vec.len() {
    let item = &vec[index];
    // ...
}
```

These can be indexed at once:

```rust
while let Some(item) = vec.get(index) {
    // ...
}
```

Particularly in `ObligationForest::process_obligations`, this mitigates
a codegen regression found with LLVM 11 (rust-lang#73526).
When a `macro_rules!` macro expands to another `macro_rules!` macro, we
may see `None`-delimited groups in odd places when another crate
deserializes the 'inner' macro. This commit 'unwraps' an outer
`None`-delimited group to avoid breaking existing code.

See rust-lang#73569 (comment)
for more details.

The proper fix is to handle `None`-delimited groups systematically
throughout the parser, but that will require significant work. In the
meantime, this hack lets us fix important hygiene bugs in macros
…omatsakis

Remove legacy InnoSetup GUI installer

On Windows the InnoSetup `.exe` installer was superseded by the MSI installer long ago. It's no longer needed.

The `.exe` installer hasn't been linked from the [other installation methods](https://forge.rust-lang.org/infra/other-installation-methods.html#standalone) page in many years. As far as I can tell the intent was always to remove this installer once the MSI proved itself. Though admittedly both installers feel very "legacy" at this point.

Removing this would mean we only maintain one Windows GUI installer and would speed up the distribution phase.

As a result of removing InnoSetup, this closes rust-lang#24397
…trait-soundness, r=nikomatsakis

Don't implement Fn* traits for #[target_feature] functions

Closes rust-lang#72012.
expand: Stop using nonterminals for passing tokens to attribute and derive macros

Make one more step towards fully token-based expansion and fix issues described in rust-lang#72545 (comment).

Now `struct S;` is passed to `foo!(struct S;)` and `#[foo] struct S;` in the same way - as a token stream `struct S ;`, rather than a single non-terminal token `NtItem` which is then broken into parts later.

The cost is making pretty-printing of token streams less pretty.
Some of the pretty-printing regressions will be recovered by keeping jointness with each token, which we will need to do anyway.

Unfortunately, this is not exactly the same thing as rust-lang#73102.
One more observable effect is how `$crate` is printed in the attribute input.
Inside `NtItem` was printed as `crate` or `that_crate`, now as a part of a token stream it's printed as `$crate` (there are good reasons for these differences, see rust-lang#62393 and related PRs).
This may break old proc macros (custom derives) written before the main portion of the proc macro API (macros 1.2) was stabilized, those macros did `input.to_string()` and reparsed the result, now that result can contain `$crate` which cannot be reparsed.

So, I think we should do this regardless, but we need to run crater first.
r? @Aaron1011
…wjasper

Provide more information on duplicate lang item error.

This gives some notes on the location of the files where the lang items were loaded from. Some duplicate lang item errors can be a little confusing, and this might help in diagnosing what has happened.

Here's an example when hitting a bug with Cargo's build-std:

```
error: duplicate lang item in crate `core` (which `rustc_std_workspace_core` depends on): `try`.
  |
  = note: the lang item is first defined in crate `core` (which `z10` depends on)
  = note: first definition in `core` loaded from /Users/eric/Proj/rust/cargo/scratch/z10/target/target/debug/deps/libcore-a764da499c7385f4.rmeta
  = note: second definition in `core` loaded from /Users/eric/Proj/rust/cargo/scratch/z10/target/target/debug/deps/libcore-5b082675aea34986.rmeta
```
…petrochenkov

Handle `macro_rules!` tokens consistently across crates

When we serialize a `macro_rules!` macro, we used a 'lowered' `TokenStream` for its body, which has all `Nonterminal`s expanded in-place via `nt_to_tokenstream`. This matters when an 'outer' `macro_rules!` macro expands to an 'inner' `macro_rules!` macro - the inner macro may use tokens captured from the 'outer' macro in its definition.

This means that invoking a foreign `macro_rules!` macro may use a different body `TokenStream` than when the same `macro_rules!` macro is invoked in the same crate. This difference is observable by proc-macros invoked by a `macro_rules!` macro - a `None`-delimited group will be seen in the same-crate case (inserted when convering `Nonterminal`s to the `proc_macro` crate's structs), but no `None`-delimited group in the cross-crate case.

To fix this inconsistency, we now insert `None`-delimited groups when 'lowering' a `Nonterminal` `macro_rules!` body, just as we do in `proc_macro_server`. Additionally, we no longer print extra spaces for `None`-delimited groups - as far as pretty-printing is concerned, they don't exist (only their contents do). This ensures that `Display` output of a `TokenStream` does not depend on which crate a `macro_rules!` macro was invoked from.

This PR is necessary in order to patch the `solana-genesis-programs` for the upcoming hygiene serialization breakage (rust-lang#72121 (comment)). The `solana-genesis-programs` crate will need to use a proc macro to re-span certain tokens in a nested `macro_rules!`, which requires us to consistently use a `None`-delimited group.

See `src/test/ui/proc-macro/nested-macro-rules.rs` for an example of the kind of nested `macro_rules!` affected by this crate.
@rustbot rustbot added the rollup A PR which is a rollup label Jul 2, 2020
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 2, 2020
@bors
Copy link
Contributor

bors commented Jul 2, 2020

⌛ Testing commit d7be1e4 with merge 63186b43f346d605af2470fcbb02f998ba7de967...

@bors
Copy link
Contributor

bors commented Jul 2, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 2, 2020
@Manishearth
Copy link
Member Author

@bors retry

This is happening pretty often, with different PRs in the rollup

@pietroalbini think this is an infra issue?

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2020
@bors
Copy link
Contributor

bors commented Jul 2, 2020

⌛ Testing commit d7be1e4 with merge 60db77216048ff47e8baa3bab92543fb5c56f0f3...

@bors
Copy link
Contributor

bors commented Jul 2, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 2, 2020
@Manishearth
Copy link
Member Author

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2020
@bors
Copy link
Contributor

bors commented Jul 2, 2020

⌛ Testing commit d7be1e4 with merge b7856f6...

@bors
Copy link
Contributor

bors commented Jul 2, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Manishearth
Pushing b7856f6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 2, 2020
@bors bors merged commit b7856f6 into rust-lang:master Jul 2, 2020
@Manishearth Manishearth deleted the rollup-0dtxnit branch July 2, 2020 15:47
@nnethercote
Copy link
Contributor

This was a perf win. Perhaps #73345 (@petrochenkov) or #73569 (@Aaron1011) is the cause?

@landaire
Copy link

landaire commented Jul 7, 2020

One of these macro-related PRs I think may end up breaking some of the more fragile proc macros in the ecosystem. We depend on one at my work which does string matching on each line of the TokenStream and performs some operations on what it finds. This macro was broken since the input TokenStream no longer has line breaks in it. I was able to determine nightly-2020-07-03 was where the breakage occurred, which I think is this rollup.

While I didn't write the macro in question I think part of the reason why this string matching approach was taken is because of the lack of resources for writing proc macros from scratch using zero dependencies. Here is a Twitter thread I found which expresses some of my own thoughts. Here's a reference to nanoserde which may serve as a good reference for anyone else wanting to correctly write a proc macro with zero dependencies: https://github.com/not-fl3/nanoserde/blob/05f40b2e1bcb97bf10104982f109a51d3a07120a/derive/src/parse.rs

While I think it's fair to say that you are not intended to write a proc macro via TokenStream string conversion, I thought it was worth mentioning.

@Manishearth
Copy link
Member Author

@landaire I do maintain a lightweight crate for doing proc macros without pulling in syn: https://docs.rs/absolution

aside from that, your comment is probably not going to have any effect if posted here

@landaire
Copy link

landaire commented Jul 7, 2020

@Manishearth thanks for the reference! I'll check it out.

aside from that, your comment is probably not going to have any effect if posted here

I wasn't really sure where the correct place for conversation was and which PR was the culprit. Our breakage is a result of assumptions not guaranteed by Rust IMO, but I wanted to raise the flag that this may cause breakage for others in the future. Would this be appropriate to file an issue over to discuss more in-depth or would the internals forum be better-suited?

@Manishearth
Copy link
Member Author

I would post on internals, this is more of a discussion thing. The string representation is deliberately not stable

@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.