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

Add lint declarative_macro_missing #966

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

miikka
Copy link
Contributor

@miikka miikka commented Oct 10, 2024

Working to implement the first item in #946: "removing a declarative macro that used to exist".

There's this case as well, but I'm not sure if it can be currently queried for:

#[macro_export]
macro_rules! inner_pub_use_will_be_removed {
    () => {};
}

pub mod inner {
    // Is it a breaking change if you remove the next line?
    pub use crate::inner_pub_use_will_be_removed;
}

@miikka miikka marked this pull request as ready for review October 13, 2024 07:03
@miikka
Copy link
Contributor Author

miikka commented Oct 13, 2024

Not sure how to address the beta failure...

The referenced type does not exist in the schema: Macro

@obi1kenobi
Copy link
Owner

obi1kenobi commented Oct 13, 2024 via email

@miikka
Copy link
Contributor Author

miikka commented Oct 13, 2024

I tried cargo update and it didn't help. I assume the point would be to update the versions of trustfall-rustdoc-adapter? It didn't touch the versions of any of those.

@obi1kenobi
Copy link
Owner

obi1kenobi commented Oct 13, 2024 via email

@obi1kenobi
Copy link
Owner

My bad, I got everything ready then just failed to cut the release 🙈 I'll have a new release up momentarily, and I'll take care of updating your PR to merge it to so I can atone for sending you down a debugging rabbit hole unnecessarily...

Excellent catch on re-exporting macros as well! To my surprise, re-exporting does seem to work: playground

#[macro_export]
macro_rules! inner_pub_use_will_be_removed {
    () => {};
}

pub mod inner {
    /// We can re-export macros at non-root paths, watch:
    /// ` ``rust
    /// use playground::inner::inner_pub_use_will_be_removed;
    ///
    /// inner_pub_use_will_be_removed! {}
    /// ` ``
    pub use crate::inner_pub_use_will_be_removed;
}

I've opened #968 to track this issue, which you are also welcome to grab if you're interested!

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together, and for the great ideas on how to test macros further!

Accepting with minor tweaks, which I'll apply since I feel bad for making you debug why the macro queries weren't working 😅

src/lints/declarative_macro_missing.ron Outdated Show resolved Hide resolved
"zero": 0,
"true": true,
},
error_message: "A publicly-visible `macro_rules` declarative macro cannot be imported by its prior name. A `#[macro_export]` may have been removed, or the macro itself may have been renamed or removed entirely.",
Copy link
Owner

Choose a reason for hiding this comment

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

Excellent diagnostic, I forgot that removing #[macro_export] is a way to cause breakage here! 🎉

test_crates/declarative_macro_missing/old/src/lib.rs Outdated Show resolved Hide resolved
test_crates/declarative_macro_missing/new/src/lib.rs Outdated Show resolved Hide resolved
@obi1kenobi obi1kenobi force-pushed the declarative-macro-missing branch from 885d709 to 6d540ee Compare October 16, 2024 00:05
@obi1kenobi obi1kenobi enabled auto-merge (squash) October 16, 2024 00:05
@obi1kenobi
Copy link
Owner

obi1kenobi commented Oct 16, 2024

The v33 rustdoc JSON adapter release was also broken due to a failure of my release automation. Whew, thank goodness for good CI that prevented the merge, or else it would have been a prod break 😅

@obi1kenobi obi1kenobi merged commit 931628b into obi1kenobi:main Oct 16, 2024
33 checks passed
@miikka miikka deleted the declarative-macro-missing branch October 16, 2024 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants