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

Visit attributes of trait impl items during AST validation #104148

Merged
merged 1 commit into from Nov 15, 2022

Conversation

fmease
Copy link
Member

@fmease fmease commented Nov 8, 2022

Fixes #104140.

This fix might not be backward compatible (in practice) since we now validate more attributes than before.

@rustbot label A-attributes

@rustbot
Copy link
Collaborator

rustbot commented Nov 8, 2022

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-attributes Area: #[attributes(..)] labels Nov 8, 2022
@petrochenkov
Copy link
Contributor

This fix might not be backward compatible since we now validate more attributes than before.

Let's run crater on this.
@bors try

@bors
Copy link
Contributor

bors commented Nov 9, 2022

⌛ Trying commit 5ef85bb with merge 692571811df96d28cfa90e2b8cdf3be7e99c44e1...

@petrochenkov petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 9, 2022
@bors
Copy link
Contributor

bors commented Nov 9, 2022

☀️ Try build successful - checks-actions
Build commit: 692571811df96d28cfa90e2b8cdf3be7e99c44e1 (692571811df96d28cfa90e2b8cdf3be7e99c44e1)

@petrochenkov
Copy link
Contributor

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-104148 created and queued.
🤖 Automatically detected try build 692571811df96d28cfa90e2b8cdf3be7e99c44e1
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🚧 Experiment pr-104148 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-104148 is completed!
📊 128 regressed and 11 fixed (247786 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Nov 10, 2022
@fmease
Copy link
Member Author

fmease commented Nov 10, 2022

I've never looked at a crater report before and don't really know what to look for. I've read through maybe a dozen crate build logs in the categories “regressed: dependencies” & “regressed: root results” but all of those failures look irrelevant/spurious to me.

@petrochenkov
Copy link
Contributor

  • legitimate regressions:
    • regressed: dependencies
      • geo-types-0.7.*
    • regressed: root results
      • geo-types-0.7.7
  • maybe legitimate regressions:
    • regressed: dependencies
      • syn-..*, const_format_proc_macros-0.2.29
    • regressed: root results
      • mistrpopo.MandelbrotAnimation.38310e3c26428ceea79a99fc6233130a53373f12
      • AOSC-Dev.p-vector-rs.c9e61c5575575d8f8396e6507b12bdaf36a41efc
      • ababup1192.elm-init.b7f79c2d4aa3c19891c0f794b8379f8e2349dd8f
      • liushuyu.aosc-findupdate.ea6022203f475fe9f5403eed921467df377d9a4d

@petrochenkov
Copy link
Contributor

Now I think you need to

  • send a fix to the geo-types crate
  • investigate what actually happens in the "maybe legitimate" cases - whether they reproduce locally with or without your patch

(Everything in the crater report is clickable, with links pointing to the relevant crates.io crates or repositories etc.)

@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 Nov 11, 2022
bors bot added a commit to georust/geo that referenced this pull request Nov 11, 2022
932: Incoming breakage: Replace an invalid `#[deprecated]` attribute with a doc comment r=michaelkirk a=fmease

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

`#[deprecated]` attributes on `impl` blocks do not have any effect and normally the deny-by-default lint `useless_deprecated` would be triggered with the message *this `#[deprecated]` annotation has no effect*.
However, there is a bug in the current version of the Rust compiler that makes it ignore the attribute entirely if it is *malformed*
which is the case for the `#[deprecated]` attached to the `From` impl for `GeometryCollection` (the version is not surrounded by double quotes).
I've replaced the attribute with a documentation comment similar to the one for the softly deprecated associated function `GeometryCollection::new_from`.

---

The compiler bug is about to be fixed (rust-lang/rust#104148) and the malformed attribute will soon become a hard error.
This future breakage was found with the help of [crater](https://github.com/rust-lang/crater) (see the [build log](https://crater-reports.s3.amazonaws.com/pr-104148/try%23692571811df96d28cfa90e2b8cdf3be7e99c44e1/reg/geo-types-0.7.7/log.txt)).

Co-authored-by: León Orell Valerian Liehr <liehr.exchange@gmx.net>
@fmease
Copy link
Member Author

fmease commented Nov 14, 2022

Legitimate Regressions

georust/geo-types: Fix sent & merged (georust/geo#932).

Maybe Legitimate Regressions

I am using cargo +<nightly|stage1> <b|t> on each cloned repository checked out at the respective commit found in the crater report where stage1 refers to the local toolchain with the patch from this PR applied. Host: x86_64-unknown-linux-gnu.

Summary: All of those regressions are spurious / could not be reproduced.

  • mistrpopo/MandelbrotAnimation: nightly & stage1: build & test pass
  • AOSC-Dev/p-vector-rs (requires libzmq installed on the system): nightly & stage1: build & test pass
  • ababup1192/elm-init: nightly & stage1: build & test pass
  • AOSC-Dev/aosc-findupdate: nightly & stage1: build passes, test fails (requires GITHUB_TOKEN env var I don't want to provide)
  • proc_qq_codegen: nightly & stage1: build & test pass
  • rust-ad-core-macros: nightly & stage1: build fails (using function pointers as const generic parameters is forbidden); crater actually reports a different compile error about proc_macro2::TokenStream (see list of ignored repos below)
  • WilliamVenner/prefer-dynamic: nightly & stage1: build & test pass; crater actually reports a different error about proc_macro2::TokenStream (see list of ignored repos below)
  • laszlo-ratesic/myepicproject: nightly & stage1: build passes, test fails (build failure: ambiguous name because of multiple potential import sources); crater actually reports a link failure (see list of ignored repos below)
  • linfeng-crypto/cro-sign-tool: stage1: build & test pass; crater actually reports compile errors like file not found for module proto_mod

Ignored

Repos skipped during local reproduction attempts as their build failures are deemed spurious by me after reading the respective build log.

ignored repos
  • trait bound proc_macro2::TokenStream: From<proc_macro::TokenStream> is not satisfied (and variations); I cannot reproduce the error; failure assumed to be spurious by me after checking the two crates rust-ad-core-macros & WilliamVenner/prefer-dynamic (see above)
    • re-parse
    • redis_serde_json
    • proc-macro-kwargs-derive
    • rasn-derive
    • sophia_rio
    • art-in.wasm-debug-symbols
  • build script failure
    • stuartZhang/scaffold-wizard
    • catboost2, catboost
  • build script failure (could not locate rustc, see also Tons of spurious failures due to build scripts failing to run rustc crater#663)
    • YuraCobain/chip8_emulator_rust
    • Zolmok/up2date
    • adamtpowell/chip8-rust
    • andrei-toterman/medial_axis_2d
    • anotak/riftwizardstats
    • arunvijayshankar/trust
    • pty_closure
    • pushgen
    • python27-sys
    • randrust
    • rate-sick-check
    • rcrawl
    • readfilez
    • remember
    • remotefs
    • rocket_http
    • rrpack-prime
    • secret-toolkit-permit
    • sheesy-tools
    • si7021
    • space-filling
    • squirrel3-rs
    • srv
    • ssite
    • starknet
    • stemma_soil_sensor
  • link failure
    • heygema/_learning-solana-buildspace-program
    • carbondesigned/ShareDeso
    • pdarden/solana-gif-gate
    • davidediak/solana-twitter
    • CeNiEi/solana-whatsapp
    • codeYouDumb97/calcSolana
    • heyAyushh/anchor-init
    • 0xdeepmehta/serialize-txs-anchor
    • QuarryProtocol/gauge
    • QuarryProtocol/quarry
    • dizvyagintsev/solana_donations
    • evanmarshall/solrand
    • yourarj/solana-anchor-zero-copy

@fmease
Copy link
Member Author

fmease commented Nov 14, 2022

@rustbot reviewer

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

@bors r+

@bors
Copy link
Contributor

bors commented Nov 14, 2022

📌 Commit 5ef85bb has been approved by petrochenkov

It is now in the queue for this repository.

@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 Nov 14, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 15, 2022
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#103439 (Show note where the macro failed to match)
 - rust-lang#103734 (Adjust stabilization version to 1.65.0 for wasi fds)
 - rust-lang#104148 (Visit attributes of trait impl items during AST validation)
 - rust-lang#104241 (Move most of unwind's build script to lib.rs)
 - rust-lang#104258 (Deduce closure signature from a type alias `impl Trait`'s supertraits)
 - rust-lang#104296 (Walk types more carefully in `ProhibitOpaqueTypes` visitor)
 - rust-lang#104309 (Slightly improve error message for invalid identifier)
 - rust-lang#104316 (Simplify suggestions for errors in generators.)
 - rust-lang#104339 (Add `rustc_deny_explicit_impl`)

Failed merges:

 - rust-lang#103484 (Add `rust` to `let_underscore_lock` example)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 47f9d89 into rust-lang:master Nov 15, 2022
@fmease fmease deleted the fix-104140 branch November 15, 2022 16:08
@cuviper cuviper added this to the 1.67.0 milestone Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: #[attributes(..)] S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE with malformed rustc_on_unimplemented on impl
6 participants