polish up the noncopyable diagnostics #66090
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
kavon
requested review from
hborla,
slavapestov,
xedin,
ahoppen,
bnbarham,
CodaFi,
DougGregor,
rintaro and
AnthonyLatsis
as code owners
kavon
force-pushed
the
noncopyable-diagnostics-cleanup
branch
from
76d7c6f to
ee63610
Compare
|
@swift-ci smoke test linux |
kavon
force-pushed
the
noncopyable-diagnostics-cleanup
branch
2 times, most recently
from
875933e to
7af33a5
Compare
kavon
changed the title
|
@swift-ci please test |
gottesmm
reviewed
May 24, 2023
include/swift/AST/DiagnosticsSIL.def
Outdated
| "'%0' consumed in closure. This is illegal since if the closure is invoked more than once the binding will be uninitialized on later invocations", (StringRef)) | ||
| "'%0' is borrowed and cannot be consumed by closure capture", (StringRef)) | ||
|
|
||
| ERROR(sil_moveonlychecker_capture_consumed_in_closure, none, |
There was a problem hiding this comment.
I looked through the patch and I think you are using this for escaping closures. I think there are two problems here:
- I think the information that the closure is escaping is important for the user to understand. We should tell the user that especially since we made the change to make non-escaping lets non-escaping. That provides a lot of clarity to the user.
- This diagnostic as shown below also occurs when a value that is captured by an escaping closure is used outside of the closure itself (the property is not flow sensitive). So the diagnostic says the use is in a closure, but it clearly isn't which is misleading.
There was a problem hiding this comment.
Seems like the main thing we need to communicate is that a mutable capture can be consumed in a closure, but an immutable one cannot. That doesn't change whether the closure is passed to an @escaping parameter or not.
kavon
force-pushed
the
noncopyable-diagnostics-cleanup
branch
from
07eb7ad to
8340753
Compare
- 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
also moves the error to the invalid partial consume. rdar://109281444
rdar://109281444
rdar://109281444
rdar://109281444
…yable binding rdar://109281444
- 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
…more consistent word tense sil_movekillscopyablevalue_* and sil_moveonlychecker_* can share diagnostics. rdar://109281444
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
kavon
force-pushed
the
noncopyable-diagnostics-cleanup
branch
from
8340753 to
03d2017
Compare
|
@swift-ci please test and merge |
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.
🚧 here be lots of find-replace 🚧
and "lvalue" with corresponding language-level notions
and probably more.
rdar://109281444