-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Track labels in the HIR #7021
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
Track labels in the HIR #7021
Conversation
crates/hir_def/src/body.rs
Outdated
| pub fn label_syntax(&self, label: LabelId) -> Result<LabelSource, SyntheticSyntax> { | ||
| self.label_map_back[label].clone() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think labels can't be synthetic (ie, we don't produce labels during desugaring), so this should return a value rather than a result. And, more generally, we shouldn't store a Result in the source map in the frist palce.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I partially based this off of patterns hence why I used this here but I think you are right, we aren't producing new labels.
matklad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although it maybe makes sense to fold basic goto definition support here as well, just to be able to implement a smoke test.
|
I already have everything done for the the entire rename and goto scheme so its all tested in a sense, I just split the work up to get smaller PRs hence this only contains the changes to HIR. |
|
bors r+ Splitting is generally good, but I personally also strongly prefer at least a hello-world test. There's no crisp and clear solutions here, ideally, it should be possible to spike the main functionality such that it has smoke tests, and then add extra bells&whistles in follow ups |
|
Fair point, I'll keep that in mind 👍 |
Groundwork for #6966