Skip to content

Hygienic identifiers#356

Closed
widberg wants to merge 7 commits into
jam1garner:masterfrom
widberg:hygienic_identifiers
Closed

Hygienic identifiers#356
widberg wants to merge 7 commits into
jam1garner:masterfrom
widberg:hygienic_identifiers

Conversation

@widberg

@widberg widberg commented Feb 20, 2026

Copy link
Copy Markdown
Contributor

This PR fixes a bug where deriving BinRead/BinWrite through a macro-expanded derive path could error in attributes like #[br(pre_assert(...))] with error[E0425]: cannot find value __binrw_generated_position_temp in this scope, help: an identifier with the same name is defined here, but is not accessible due to macro hygiene. This was revealed when I tried using derive-aliases, but it can happen with normal macro_rules!, see the tests. This PR makes the __binrw_generated_position_temp-style IdentStrs use Span::call_site() in to_ident and to_spanned_tokens while leaving the span untouched for other IdentStrs, which fixes the error for me. If there is another way you want this issue solved, I can rework it.

Also, I considered adding something like this to simplify the sanitization file:

ident_str! {
    pub(crate) hygienic POS = "__binrw_generated_position_temp";
}

But it seemed like overkill for a couple of identifiers. Let me know if you want me to put that back in or do something else.

Long-term, I think we should be more careful with spans; I saw some TODOs related to this. Even then, it's probably a good idea to keep these changes.

Finally, I fixed some new warnings and clippy lints to keep CI happy, but the nightly UI tests will probably still fail.

@widberg

widberg commented Feb 20, 2026

Copy link
Copy Markdown
Contributor Author

Looks like the 1.70 test is failing because syn bumped their rust-version to 1.71. Should we bump ours, too?

@csnover

csnover commented Feb 21, 2026

Copy link
Copy Markdown
Collaborator

Looks like the 1.70 test is failing because syn bumped their rust-version to 1.71. Should we bump ours, too?

Oh, an easy question that my brain can handle! Yes. :-)

@widberg widberg force-pushed the hygienic_identifiers branch 3 times, most recently from 77bb652 to 6be6143 Compare February 21, 2026 03:29
@widberg widberg force-pushed the hygienic_identifiers branch from 6be6143 to 45c1a2f Compare February 21, 2026 03:45
@widberg

widberg commented Feb 21, 2026

Copy link
Copy Markdown
Contributor Author

Ended up bumping it up to 1.76 because of trybuild. That should only be a dev dependency; I could try to maintain the 1.71 MSRV despite that, but it might get complicated, and 1.76 is already pretty old, so we'd likely have to bump again soon if we don't do it now. The nightly UI tests are still failing, expectedly. Do we want to disable the UI tests for nightly in CI if it just adds noise?

@csnover

csnover commented Feb 21, 2026

Copy link
Copy Markdown
Collaborator

Do we want to disable the UI tests for nightly in CI if it just adds noise?

Just run TRYBUILD=overwrite cargo +nightly test, git add -u, git diff --staged to make sure it looks sane, and commit.

Once all the span stuff finally gets to the stable rustc the UI tests can run against stable instead. Of course with my limited ability to focus on any of this, and having no other co-maintainer, it might not make much of a difference in terms of how frequently the UI tests need to be updated :-)

@widberg widberg force-pushed the hygienic_identifiers branch from 45c1a2f to 7e798e4 Compare February 21, 2026 05:00
@widberg

widberg commented Feb 21, 2026

Copy link
Copy Markdown
Contributor Author

Oh my bad, I thought nightly and stable were disagreeing about the output for the same files.

@widberg widberg closed this May 21, 2026
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