[5.9🍒] polish up the noncopyable diagnostics #66124
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…Use. I think this was a mistake from when I changed implementations to use pure scalar bit processing rather than processing all at once. Instead of just updating the internal found resulting uses array, we were appending to it. Some notes: 1. This actually did not break anything semantically in the move checker since we do not use this array in the caller in anyway. We just use it internally in the routine to first lookup the current state which we then process in the routine. That being said, this API is written such that a user /could/ do that and we want to allow for users to be able to do that so that we match what PrunedLiveness does. 2. This could cause memory corruption due to iterator invalidation if by appending we caused the SmallVector to reallocate as we iterated over the array. So to fix this I did the following: a. I changed the push_back to be an assignment. b. I removed llvm::enumerate just out of paranoia if the assignment could potentially cause iterator invalidation. The given test exercises this code path and with the old behavior would crash with asan or guard malloc. rdar://109673338 (cherry picked from commit 5bef851)
NOTE: This does not affect normal parameters since normal parameters that are noncopyable never have default access semantics since the user is forced to specify either borrow or consume. This is incontrast to implicit parameters like the newValue of a setter. rdar://109726282 (cherry picked from commit 82c645d)
…we do for loadable types. rdar://109686285 (cherry picked from commit ec28aa4)
- replaces "move-only" terminology with "noncopyable" - replaces compiler jargon like "guaranteed parameters" and "lvalue" with corresponding language-level notions - simplifies diagnostics about closures. and probably more. rdar://109281444 (cherry picked from commit 31aa2f7)
also moves the error to the invalid partial consume. rdar://109281444 (cherry picked from commit d3730d8)
rdar://109281444 (cherry picked from commit 3149102)
rdar://109281444 (cherry picked from commit 90077ab)
rdar://109281444 (cherry picked from commit 616395c)
…yable binding rdar://109281444 (cherry picked from commit e9e6cda)
- refer to a "consuming use" as simply a "consume", to reserve "use" for non-consuming uses. - refer to "non-consuming uses" as just a "use". - don't call it a "user defined deinit" and instead a "deinitializer" to match Sema - be specific about what binding a closure is capturing that is preventing consumption. rdar://109281444 (cherry picked from commit 667459e)
…more consistent word tense sil_movekillscopyablevalue_* and sil_moveonlychecker_* can share diagnostics. rdar://109281444 (cherry picked from commit 71763a1)
this also fixes a bug where sometimes we simply emit 'consumed here' twice and other times we'd said 'other consume here' for the same "consumed more than once" message. so I went through and changed all of the 2nd consumes into "consumed again". rdar://109281444 (cherry picked from commit 03d2017)
|
@swift-ci please test |
jckarter
approved these changes
May 25, 2023
|
@kavon is currently sick and I haven't tested my PR that this is based off of, so I am going to take over this PR and use it for my CCC as well. |
tbkka
approved these changes
May 25, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MichaelG: This PR contains a few different batched pieces of work. The first 3 patches are patches by me that I included specific CCC for below. The last series of patches is one piece of work by Kavon that he included CCC for.
---- CCC by MichaelG
Commit: 5372ce8
• Description: Fixes a thinko that caused a memory bug from iterator invalidation.
• Risk: Low. The thinko as shown in the commit message never actually affected program behavior and just could cause iterator invalidation. So just goodness.
• Original PR: #66065
• Reviewed By: @jckarter
• Testing: Added test that triggers the bad behavior
• Resolves: rdar://109673338
Commit: 780d88c
• Description: [move-only] Fix emission of addressonly noncopyable setter new values.
• Risk: Low. Only affects noncopyable types.
• Original PR: #66084
• Reviewed By: @jckarter
• Testing: Added test that triggers the bad behavior
• Resolves: rdar://109726282
Commit: 780d88c
• Description: [move-only] Ban destructuring of noncopyable address only types like we do for loadable types.
• Risk: Low. Only affects noncopyable types.
• Original PR: #66092
• Reviewed By: @jckarter
• Testing: Added tests that showcase the behavior.
• Resolves: rdar://109686285
---- CCC By Kavon
• Description: Updates diagnostics to refer to "noncopyable" types instead of move-only. Simplifies and makes wording of many diagnostics for noncopyable types / ownership in general clear and more consistent.
• Risk: Low. This is a diagnostics focused change and doesn't introduce any new errors. In one case, an error is moved from the definition to the offending use, and the note previously emitted on the offending use is dropped since it's then not needed.
• Original PR: #66090
• Reviewed By: @jckarter
• Testing: tests included
• Resolves: rdar://109281444