-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Rename outer_expn -> expn #146100
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
Rename outer_expn -> expn #146100
Conversation
This naming has always confused me. I'm not sure what "outer" is supposed to mean.
r? @SparrowLii
rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r?
to explicitly pick a reviewer
Some changes occurred to the CTFE machinery
Some changes occurred in src/tools/clippy
cc @rust-lang/clippy
Some changes occurred in compiler/rustc_ast_lowering/src/format.rs
cc @m-ou-se
The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.
cc @BoxyUwU, @jieyouxu, @Kobzol, @tshepang
Some changes occurred in check-cfg diagnostics
cc @Urgau
Some changes occurred to MIR optimizations
cc @rust-lang/wg-mir-opt
Some changes occurred in coverage instrumentation.
cc @Zalathar
r? petrochenkov
SyntaxContext
potentially consists of many expansions, organized as a "child -> parent" linked list, outer_expn
returns only one outermost expansion out of the whole set.
The new naming gives an impression that a span / syntax context only has one expansion.
outer_expn
returns only one outermost expansion out of the whole set.
Couldn't it also be the innermost or anything in between?
I think I see where you're coming from now. "outer" means "this expansion which might have child expansions". It confused me because I thought it meant "get the parent expansion relative to this expansion". But that is parent_callsite
. As a thought experiment, why not call it inner_expn
in order to communicate that there could be parent expansions?
The new naming gives an impression that a span / syntax context only has one expansion.
To me it just says "give me the expansion of this span" without making any implications about parent or child expansions.
Anyways, this could just be me.
Couldn't it also be the innermost or anything in between?
You can call it the innermost if you count from the opposite side, but certainly not something in between.
It's id(n)
in "Example 1" in https://rustc-dev-guide.rust-lang.org/macro-expansion.html#the-macro-definition-hierarchy.
To me it just says "give me the expansion of this span" without making any implications about parent or child expansions.
A span (or rather SyntaxContext
) doesn't have the expansion, a span is a list of expansions, that's the point.
It's different from e.g. parent_callsite
stuff which is uniquely determined by ExpnId
, SyntaxContext
is not uniquely determined by its outer ExpnId
as the dev guide explains.
I see what happened. I was thinking of SyntaxContext as being an ID for just the outer(?) expansion rather than representing the whole chain. With that shift, all the methods on SyntaxContext make much more sense now.
You can call it the innermost if you count from the opposite side
I think this is part of what stalled my understanding. I think of outer_expn
as "go to the innermost expansion". The dev guide even uses the word "innermost" in this way at one point.
I can change this PR to just add some docs. Or what do you think of renaming to last_expn
or inner_expn
or tail_expn
?
Let's just keep the current naming to avoid churn.
Doc improvements are always welcome, though.
This naming has always confused me. I'm not sure what "outer" is supposed to mean. Zulip thread.