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

regression: a value of type HashMap<Pulse, u64> cannot be built from #135669

Open
BoxyUwU opened this issue Jan 18, 2025 · 18 comments
Open

regression: a value of type HashMap<Pulse, u64> cannot be built from #135669

BoxyUwU opened this issue Jan 18, 2025 · 18 comments
Labels
A-edition-2024 Area: The 2024 edition disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. I-edition-triaged Issue: This issue has been reviewed and triaged by the Edition team. I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 18, 2025

[INFO] [stderr]    Compiling advent-2023 v0.1.0 (/opt/rustwide/workdir)
[INFO] [stdout] error[E0277]: a value of type `HashMap<Pulse, u64>` cannot be built from an iterator over elements of type `&(Pulse, {integer})`
[INFO] [stdout]    --> src/bin/20/main.rs:278:88
[INFO] [stdout]     |
[INFO] [stdout] 278 |           assert_eq!(config.counts, [(Pulse::Low, low), (Pulse::High, high)].into_iter().collect());
[INFO] [stdout]     |                                                                                          ^^^^^^^ value of type `HashMap<Pulse, u64>` cannot be built from `std::iter::Iterator<Item=&(Pulse, {integer})>`
[INFO] [stdout] 279 |       } }
@BoxyUwU BoxyUwU added regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 18, 2025
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 18, 2025
@compiler-errors
Copy link
Member

This one seems to have regressed in:

83965ef #133274

cc @ehuss, tho i'll double check when i'm done bisecting the rest

@theemathas
Copy link
Contributor

theemathas commented Jan 18, 2025

Minimized (zipped for your convenience: foo.zip):

src/lib.rs

// This crate is in edition 2021

bar::create_macro! {{
    // This code works on edition 2021, but not edition 2018
    let _ = [1i32].into_iter().collect::<Vec<i32>>();
}}
create_fn!();

bar/src/lib.rs

// This crate is in edition 2018

#[macro_export]
macro_rules! create_macro {
    ($body:tt) => {
        macro_rules! create_fn {
            () => {
                fn a() {
                    $body
                }
            };
        }
    };
}
Cargo.toml
[workspace]
members = ["bar"]

[package]
name = "foo"
version = "0.1.0"
edition = "2021"

[dependencies]
bar = { path = "bar" }
bar/Cargo.toml
[package]
edition = "2018"
name = "bar"
version = "0.1.0"

In stable, the function body is compiled as edition 2021. In beta, it is compiled as edition 2018.

For context: This is the intended usage pattern of the parameterized_test crate.

@ehuss
Copy link
Contributor

ehuss commented Jan 20, 2025

Hm, yea, I can see how that doesn't look to be working quite as I intended. I would expect substitutions to retain whatever edition the input has, but it makes sense that the macro_rules compiler is ignoring that.

I would be fine reverting #133274 if that is what people want. It did not have a high importance for edition compatibility. I don't recall how many crates would fail in the 2024 edition without it, though. I think it should eventually be fixed, but may require deeper changes if it is possible.

@ehuss ehuss added the A-edition-2024 Area: The 2024 edition label Jan 20, 2025
@workingjubilee workingjubilee added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 21, 2025
@traviscross
Copy link
Contributor

@rustbot labels -T-libs +T-lang +I-lang-nominated

Let's nominate to sign off on this regression. I talked this one over with @ehuss, and the conclusion we came to is that it's probably best to just accept this if the breakage looks as minimal as what we're seeing here.

Keeping the #133274 PR is helpful to the edition migration, and more broadly, this behavior -- while itself not right either -- at least gives the caller the ability to fix this problem. What it does, essentially, is to leak out to the caller the edition of the crate that defines the macro, but we have of course other places that's also true, e.g.:

@rustbot rustbot added I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 21, 2025
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 23, 2025
@nikomatsakis
Copy link
Contributor

I think the model is this---

  • Tokens have spans and those spans are preserved through substitution in a macro.
  • Spans have editions.
  • In this case, the $body would preserve the edition info on the body expression --
  • but what changed with Use edition of macro_rules when compiling the macro #133274 is that the fn a tokens get the span of the edition?

And then the question I don't know is:

  • what tokens control which inference behavior we use here? Presumably it's the fn token?

Or is my mental model broken here somehow...? Are the editions of the pasted-in tokens being changed?

@nikomatsakis
Copy link
Contributor

@theemathas do you know whether this code would error (building on your example)...

// code in Edition 2024
bar::create_macro! {{
    let gen = 3; // <-- ERROR
}}
create_fn!();

...my expectation is that it will, because the span of the gen token would be preserved and would carry the 2024 Edition information. This is true even though the macro is defined in Rust 2018.

@ehuss
Copy link
Contributor

ehuss commented Jan 29, 2025

@nikomatsakis Your example with let gen = 3; will compile successfully on nightly, but not stable (due to #133274). The bar crate is edition 2018. The create_fn macro is compiled with the edition of bar, and gen is OK to be used there. The important bit relevant to this issue is that when compiling a macro_rules macro, it is somehow overwriting the edition of the substitutions.

Will follow up here shortly with some more information.

@nikomatsakis
Copy link
Contributor

@ehuss I see -- that is surprising to me (that it overwrites the edition of the substitutions). It is not my mental model at all. I'm trying to remember now if we decided that ultimately that was better at some point or what.

That said, if we do that, I think that the model of "use the edition of the crate definition" feels better than "edition of the crate consumer".

@theemathas
Copy link
Contributor

@nikomatsakis

what tokens control which inference behavior we use here?

This isn't actually "inference". See https://doc.rust-lang.org/edition-guide/rust-2021/IntoIterator-for-arrays.html

The token used is the into_iter token. See #86539 (comment)

@ehuss
Copy link
Contributor

ehuss commented Jan 29, 2025

what tokens control which inference behavior we use here? Presumably it's the fn token?

I don't think there is a general rule of thumb to describe which tokens affect edition behavior. It depends on the particular feature.

For let gen = 3, the edition of the gen token is used to determine the edition behavior.

For let _ = [1i32].into_iter().collect::<Vec<i32>>();, the edition of the into_iter token is the one that determines the edition behavior.


To try to explain the example above a little more explicitly, let's look at the behavior on stable (before #133274). foo is 2021, bar is 2018.

  1. When bar is built, it has a macro called create_macro with a bunch of spans equal to 2018.
  2. When foo calls create_macro, it expands to a local macro called create_fn, and create_fn has the local edition (2021) (because it uses the local crate's edition when compiling a macro).
  3. The input (let _ = …) has the local edition (2021).
  4. When foo calls create_fn, it expands to the function with all tokens at edition 2021.
  5. Compilation succeeds.

After #133274, it is:

  1. Same
  2. When foo calls create_macro, it expands to a local macro called create_fn, and create_fn is created with the edition of create_macro which is 2018 (because Use edition of macro_rules when compiling the macro #133274 changed it so that it uses the edition of the macro definition).
  3. (Same) The input (let _ = …) has the local edition (2021).
  4. Because the input was embedded in create_fn, when create_fn was compiled the input's spans ended up with 2018. I believe this is happening around here.
  5. Compilation fails.

Note that there is something really wonky here which I don't really understand. I don't think it is related to this issue, but it is weird looking. I unfortunately don't remember the context from #84429.

(For the record, I still don't fully understand expansion contexts, and so this is just the best interpretation I have.)

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 1, 2025 via email

ehuss added a commit to ehuss/rust that referenced this issue Feb 3, 2025
This adds tests to check the behavior of how nested macro_rules
definitions work across edition boundaries. This covers a change in
behavior due to rust-lang#133274.

See rust-lang#135669
ehuss added a commit to ehuss/rust that referenced this issue Feb 3, 2025
This adds tests to check the behavior of how nested macro_rules
definitions work across edition boundaries. This covers a change in
behavior due to rust-lang#133274.

See rust-lang#135669
@ehuss
Copy link
Contributor

ehuss commented Feb 3, 2025

Posted #136509 to at least add a test that would have caught this change in behavior.

ehuss added a commit to ehuss/rust that referenced this issue Feb 3, 2025
This adds tests to check the behavior of how nested macro_rules
definitions work across edition boundaries. This covers a change in
behavior due to rust-lang#133274.

See rust-lang#135669
ehuss added a commit to ehuss/rust that referenced this issue Feb 3, 2025
This adds tests to check the behavior of how nested macro_rules
definitions work across edition boundaries. This covers a change in
behavior due to rust-lang#133274.

See rust-lang#135669
jhpratt added a commit to jhpratt/rust that referenced this issue Feb 4, 2025
…r=jieyouxu

Add tests for nested macro_rules edition behavior

This adds tests to check the behavior of how nested macro_rules definitions work across edition boundaries. This covers a change in behavior due to rust-lang#133274.

See rust-lang#135669
fmease added a commit to fmease/rust that referenced this issue Feb 5, 2025
…r=jieyouxu

Add tests for nested macro_rules edition behavior

This adds tests to check the behavior of how nested macro_rules definitions work across edition boundaries. This covers a change in behavior due to rust-lang#133274.

See rust-lang#135669
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 5, 2025
Rollup merge of rust-lang#136509 - ehuss:nested-macro-rules-edition, r=jieyouxu

Add tests for nested macro_rules edition behavior

This adds tests to check the behavior of how nested macro_rules definitions work across edition boundaries. This covers a change in behavior due to rust-lang#133274.

See rust-lang#135669
@traviscross traviscross removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 5, 2025
@traviscross
Copy link
Contributor

We're deciding to accept this one-crate breakage and keep the behavior in the PR.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Feb 5, 2025

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 5, 2025
@tmandry
Copy link
Member

tmandry commented Feb 5, 2025

@rfcbot reviewed

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 5, 2025
@rfcbot
Copy link

rfcbot commented Feb 5, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

@traviscross
Copy link
Contributor

Checking @nikomatsakis' box per his comment in #133081 (comment):

As I said in the meeting:

I consider this the "least bad" option. We ought to explore how to preserve editions for interpolated token trees in macro expansion. However, we ought to do that after this edition has settled down. Seems like a good task for the newly spawned edition subteam. =)

@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 5, 2025
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Feb 6, 2025
Add tests for nested macro_rules edition behavior

This adds tests to check the behavior of how nested macro_rules definitions work across edition boundaries. This covers a change in behavior due to rust-lang/rust#133274.

See rust-lang/rust#135669
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 10, 2025

Whoops :) Thanks @traviscross and @ehuss for redirecting my comments.

@traviscross traviscross added the I-edition-triaged Issue: This issue has been reviewed and triaged by the Edition team. label Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. I-edition-triaged Issue: This issue has been reviewed and triaged by the Edition team. I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests