-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Stabilize if let
guards (feature(if_let_guard)
)
#141295
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (similarly using a random file to get an inline comment) Could you distill the contents of the "Edition Considerations" section? This section needs to say which editions this will be stable in and why, which should be concise. I'm having a very tough time following wording like "...designed to integrate seamlessly across all Rust editions" when there isn't a simple statement like "x will be stable in [all editions][edition xxxx and later]". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, i just later realise that we didnt tested this in 2015 and 2018, i will update this block sure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (from the top post)
Testing the current design is necessary, but it's much more important that you convince us why the current design is correct and why we don't need to iterate further - which no test in the world can do :). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest, I’m still trying to get a better grasp of what exactly counts as the “current design” in this context — whether it refers mostly to syntax, semantics, implementation details (like MIR/lowering), or a combination of all of these. I’ve mainly been focusing on making sure things work and are well-tested, but I now realize that’s not the same as being able to clearly explain why the design itself is correct and stable I’d really appreciate any pointers or examples of what kind of justification would be most helpful for the team here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The things that matter here are mostly syntax and semantics, since that is the part we won't be able to change in the future. Lowering can come a bit into play here since drop order interacts with MIR. As some twocents, the things you may want to consider mentioning are things like consistency with existing language behavior and anything that might be a "gotcha" for users. For example, can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the clarification — that helps a lot Regarding the syntax and semantics: the final form of the syntax was actually proposed back in the original RFC from 2018, so in terms of syntax, things have been stable and intentional from the start As for semantics as far as I concerned, the semantics between using I've tested drop order for LHS and RHS cases and they are identical There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I don't expect syntax to change, it's fine to say that it is a logical extension to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, yes they are identical, if we change match ... {
<pat> if <guard> => { <arm> }
_ => {}
} in tests to match ... {
<pat> => if <guard> {
<arm>
},
_ => {}
} nothing will change in drop order There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think Nandi may be asking for a more first principals approach, e.g. does the compiler source use the same paths for both so we have a better idea that the results will always be identical? Or at least pointing to an existing test (or a new one) that shows the two patterns generate the same MIR in all editions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I will add test that tests drop order between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please take a look at this test, if this is ok There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Nice! That's all I wanted to make sure, this test looks ok to me. Actually that can't be fully true because |
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.
Question (forcing inline comment): could you provide more details on the test coverage of this language feature? The current description is quite vague, which makes it really hard for FCP reviewers (lang or compiler) to properly determine if this language feature has sufficient test coverage esp. surrounding temporary drop order, scoping, interactions with potentially other parts of the language (like name res, attributes, macros, you name it).
That's all well and good, but could you please correlate which tests actually exercises those aspects? I'm thinking implementation and language aspects like:
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.
like it was made in #132833
tests section?
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.
Yes please. That makes it much easier to determine what is and is not exercised.
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.
Sure I will explain existed tests! But... How do I check debuginfo
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.
Hm, I'd hold off on debuginfo for now.
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.
Well, I've added tests section that covers most part of tests and explain what are they testing, about the debuginfo, all that came to my mind to test is check is check if local variables corretly shows inside match arm
I guess it's fine, is there anything we can check in debuginfo please let me know