Skip to content

Conversation

@yaahc
Copy link
Member

@yaahc yaahc commented Oct 20, 2025

currently mostly a skeleton of a draft so we can collaboratively massage it into shape more easily before filling in with proper reference verbiage.

hoping to take a significant chunk out of #568

@traviscross
Copy link
Contributor

Thanks for posting. Lot of good here. The main thing I'd suggest, by way of helping to sharpen this up, would be to try to write a concise example after each claim, in as many cases as that might make sense, that demonstrates that the claim is true (and would not pass if the claim were false). Aside from the intrinsic benefit of having such examples, I think this might help to focus the text on the language-level effects. (It's not surprising, given the good research you've been doing, that some bits of this currently have some "description of the implementation" flavor.)

Copy link
Member Author

@yaahc yaahc left a comment

Choose a reason for hiding this comment

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

a fair number of comments are also contained inline in the documents

@rustbot

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Oct 21, 2025
@traviscross
Copy link
Contributor

@ehuss and I are looking at this together on the lang-docs office hours call, and we just wanted to express our appreciation to @petrochenkov for having been so responsive with @yaahc on working out the details here. This is a chapter that we've long wanted to exist, and we're thrilled and appreciative that @yaahc is digging in to shape this up.

@yaahc yaahc force-pushed the name-res branch 6 times, most recently from 5397d08 to 1bd3afe Compare October 29, 2025 21:29
@yaahc yaahc force-pushed the name-res branch 9 times, most recently from 80fd707 to 570bedd Compare November 7, 2025 22:43
@rustbot

This comment has been minimized.

@traviscross traviscross force-pushed the name-res branch 2 times, most recently from d87f2d8 to db220a9 Compare December 9, 2025 23:31
@yaahc yaahc force-pushed the name-res branch 7 times, most recently from d4d235d to 122378f Compare December 15, 2025 18:11
@rustbot

This comment has been minimized.

ambig!(); // ERROR: `ambig` is ambiguous.
```

This also applies to imports, so long as the innermost candidate for the invocation of name is from within a macro expansion.
Copy link
Contributor

Choose a reason for hiding this comment

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

We were puzzling over this sentence and were wondering if you could clarify something.

(The phrase "the invocation of name" seemed out of place here, and we are assuming this should be "the name", since there isn't an invocation here.)

This ambiguities section starts off talking about an ambiguity involving macro invocations and expanded macros. But then this sentence seems to say, "oh and the ambiguity applies to imports, too.". Am I understanding it correctly that the rule here applies to macro invocations and imports? Is there a reason the start of the rule leaves out the "and imports" part?

Copy link
Member Author

Choose a reason for hiding this comment

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

The "invocation" language came up because of this function and how it refers to the site the name being resolved came from as an invocation regardless of whether it was an import or a macro. I wasn't really sure if this was the more proper term or not, elsewhere in the reference I've used language like "name is used" to the same effect.

I just reread the beginning of this ambiguity section and at least the way I read and meant it it's not talking about this as only applying to macros. Whether or not the invocation is a macro or a use decl there have to be macro expansions involved in order for this lint to trigger. I did my best to make the language generic in the beginning and talk about candidates without specifically saying imports or macro definitions as the sources of those candidate bindings.

@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@traviscross traviscross force-pushed the name-res branch 4 times, most recently from 0a93b28 to 934f5ad Compare December 17, 2025 01:14
yaahc and others added 3 commits December 17, 2025 01:15
Co-authored-by: Vadim Petrochenkov <vadim.petrochenkov@gmail.com>
Co-authored-by: Eric Huss <eric@huss.org>
Co-authored-by: Tshepang Mbambo <hopsi@tuta.io>
Co-authored-by: Travis Cross <tc@traviscross.com>
Many earlier passes of revisions have been squashed into the chapter
itself.  These edits are the final pass.
We're not certain what this rule means; we've run various tests and
could not confirm our interpretation of the rule.  E.g. (thanks to
ehuss for this):

```rust
use pm::{WithHelperAttr, helper};
#[derive(WithHelperAttr)]
#[helper]
#[derive(helper)] // ERROR: expected derive macro, found derive helper attribute `helper`
struct S;
```

Interestingly, this works and calls the attribute macro:

```rust
use pm::{WithHelperAttr, helper};
#[derive(WithHelperAttr, helper)]
#[helper]
struct S;
```

So that we can merge the rest, let's remove this rule for now and add
it back in future work.
@traviscross traviscross added this pull request to the merge queue Dec 17, 2025
@traviscross
Copy link
Contributor

traviscross commented Dec 17, 2025

Thanks @yaahc. Great work here, and great working with you on it. Thanks to @petrochenkov for the critical support.

Merged via the queue into rust-lang:master with commit 4d81027 Dec 17, 2025
5 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: The marked PR is awaiting review from a maintainer label Dec 17, 2025
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.

7 participants