fix(lang): qualify bare error! calls in require macros#4639
fix(lang): qualify bare error! calls in require macros#4639at264939-ctrl wants to merge 2 commits into
Conversation
|
@at264939-ctrl is attempting to deploy a commit to the OtterSec Team on Vercel. A member of the Team first needs to authorize it. |
|
Is there no one here for review? |
acheroncrypto
left a comment
acheroncrypto
left a comment
There was a problem hiding this comment.
Closes #4049
Thanks for the PR, but these changes absolutely do not resolve #4049, as that's a much larger issue affecting a lot more places.
Refer to #4157 (comment) to understand why we do not prefer to have the :: prefix everywhere.
Another example: #4476 (comment) would not be possible with the :: prefix, as you'd need to add anchor-lang dep to the anchor-lang-error crate, which would result in a circular dependency problem.
The term "hygienic" was originally coined in order to emphasize the importance of name clashes. What's the chance of an unintentional clash between anchor_lang a user-defined item's name?
This approach sounds nice (who would want an "unhygienic" macro, right?), but in reality it just results in unnecessary implementation detail leakage by forcing every downstream user to know about the dependencies used and add them as needed with correct features, even when the dependency is never directly used by downstream projects (e.g. this was a common problem with the zero copy feature).
On the other hand, Anchor was initially built with the assumption that anchor_lang::prelude::* was always available. This was also not the best approach, so we later stopped making that assumption (for the most part) and started using full paths (without the :: prefix).
Could you change the PR to fix those issues instead? For example, some of the declarative macros require error! to always be in scope. We can change them to anchor_lang::error!. You don't have to fix all of them in this PR.
|
|
||
| // Regression test for https://github.com/otter-sec/anchor/issues/4049 | ||
| // | ||
| // Verifies that macros expand correctly when anchor_lang is accessed through a | ||
| // crate alias. Prior to the fix each macro hardcoded the literal tokens | ||
| // `anchor_lang::` in its expansion, causing a name resolution failure whenever | ||
| // the crate was accessed under a different identifier. | ||
| mod crate_hygiene { | ||
| use anchor_lang as al; | ||
|
|
||
| fn check_eq(a: u64, b: u64) -> al::Result<()> { | ||
| al::require_eq!(a, b); | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn check_neq(a: u64, b: u64) -> al::Result<()> { | ||
| al::require_neq!(a, b); | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn check_gt(a: u64, b: u64) -> al::Result<()> { | ||
| al::require_gt!(a, b); | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn check_gte(a: u64, b: u64) -> al::Result<()> { | ||
| al::require_gte!(a, b); | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn check_require(cond: bool) -> al::Result<()> { | ||
| al::require!(cond, al::error::ErrorCode::ConstraintRaw); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn require_eq_via_alias() { | ||
| assert!(check_eq(5, 5).is_ok()); | ||
| assert!(check_eq(5, 6).is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn require_neq_via_alias() { | ||
| assert!(check_neq(5, 6).is_ok()); | ||
| assert!(check_neq(5, 5).is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn require_gt_via_alias() { | ||
| assert!(check_gt(6, 5).is_ok()); | ||
| assert!(check_gt(5, 5).is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn require_gte_via_alias() { | ||
| assert!(check_gte(5, 5).is_ok()); | ||
| assert!(check_gte(6, 5).is_ok()); | ||
| assert!(check_gte(4, 5).is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn require_via_alias() { | ||
| assert!(check_require(true).is_ok()); | ||
| assert!(check_require(false).is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn err_via_alias() { | ||
| let e: al::Result<()> = al::err!(al::error::ErrorCode::ConstraintRaw); | ||
| assert!(e.is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn source_via_alias() { | ||
| let src = al::source!(); | ||
| assert!(!src.filename.is_empty()); | ||
| assert!(src.line > 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Added a
crate_hygienetest module in lang/tests/macros.rs that aliases
anchor_lang as al and exercises every affected macro through the alias.
This serves as both a compile-time guard (macros with the old hardcoded
paths would fail to compile in this configuration) and a runtime check
that the correct error codes are returned.
These tests do not check the crate aliasing issue at all. You could replace every $crate with anchor_lang, and they would all still pass.
The usefulness of these tests is also questionable. If you renamed your dependency for whatever reason, you could just import it as anchor_lang in the scope that it's used. Why would this ever block anyone?
in their custom-error-code arms, which implicitly required error! to be in scope via the prelude glob import. Qualify these with anchor_lang:: so they resolve without relying on any particular import at the call site.
at264939-ctrl
force-pushed
the
fix/macro-crate-hygiene-4049
branch
from
f714335 to
006917c
Compare
Thanks for the review. I reverted the proc-macro changes and removed the alias tests. This revision only updates the declarative macros that depended on The goal here is to address the scoped |
acheroncrypto
left a comment
acheroncrypto
left a comment
There was a problem hiding this comment.
It looks much better. Thanks for updating!
Could you also update the CHANGELOG and the PR title to reflect this change?
|
LGTM once PR title and body are updated |
at264939-ctrl
changed the title
OK, I've done that. I've updated the PR title, body, and the CHANGELOG. Regarding the failing CI checks: I've noticed they are failing due to a transient GitHub API error (Failed to parse Github response) while installing platform-tools. This seems unrelated to my changes. Could you please trigger a re-run of the checks? Thanks |
3 participants
Problem
Some declarative macros in
anchor-lang(likerequire_eq!,require_gt!, etc.) relied on theerror!macro being available in the caller's scope via the prelude glob import. This caused issues when the prelude wasn't imported.Fix
Qualified all bare
error!calls insidemacro_rules!in lang/src/lib.rs withanchor_lang::error!. This ensures the macros are self-contained and resolve correctly regardless of the caller's imports.