[Concurrency] if let value on async value should be banned (not crash) #74475
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
ktoso
requested review from
hborla,
xedin,
slavapestov,
ahoppen,
bnbarham,
CodaFi,
DougGregor,
hamishknight and
rintaro
as code owners
ktoso
commented
Jun 17, 2024
ktoso
force-pushed
the
wip-async-get-if-let
branch
3 times, most recently
from
d078c46 to
81f601f
Compare
|
@swift-ci please smoke test |
ktoso
force-pushed
the
wip-async-get-if-let
branch
from
3a2c316 to
3fdd17e
Compare
|
@swift-ci please smoke test |
ktoso
force-pushed
the
wip-async-get-if-let
branch
from
3fdd17e to
05e1cc1
Compare
|
@swift-ci please smoke test |
|
I'll look into removing the special casing previously added for the |
ktoso
force-pushed
the
wip-async-get-if-let
branch
2 times, most recently
from
74a410f to
5a061ae
Compare
The issue is that the shorthand if let syntax injects an implicit expression: swiftlang#40694 in ParseStmt and that the 'diagnoseUnhandledAsyncSite' explicitly avoids reporting errors in implicit expressions. This change is that we don't mark the implicit declref code emitted by the `if let prop` as implicit anymore, and this way the reporting works out as expected. Added some tests covering this as well as properly erroring out for the nonexistent syntax of shortand + awaiting which doesn't exist, and we properly error on it. Resolves rdar://126169564
ktoso
force-pushed
the
wip-async-get-if-let
branch
from
5a061ae to
0d3a4b4
Compare
|
@swift-ci please smoke test |
|
Thanks for the reviews @ahoppen @hamishknight ! |
| // expected-warning@-1 {{immutable value 'result' was never used; consider replacing with '_' or removing it}} | ||
| // expected-note@-2 {{reference to async let 'result' is 'async'}} | ||
| if let result = result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{19-19=await }} | ||
| // expected-warning@-1 {{value 'result' was defined but never used; consider replacing with boolean test}} | ||
| // expected-note@-2 {{reference to async let 'result' is 'async'}} | ||
| if let result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{16-16= = await result}} | ||
| if let result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{10-10=await }} |
There was a problem hiding this comment.
Hmm this doesn't quite seem right, if let await result is invalid, right? Looks like this fix-it logic is relying on the implicitness to detect the shorthand binding, maybe we need a bit to track this after all?
There was a problem hiding this comment.
Hm true that’s wrong… I’ll look if there’s an easy fix… would like to prevent the crash asap but let me look at the bad fixit as well
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.
The issue is that the shorthand if let syntax injects an implicit expression: #40694 in ParseStmt and that the 'diagnoseUnhandledAsyncSite' explicitly avoids reporting errors in implicit expressions.
This patch "peeks" at the expression to notice this case, however perhaps we should mark this implicit expression in ParseStmt and then detect it explicitly.
Added some tests covering this as well as properly erroring out for the nonexistent syntax of shortand + awaiting which doesn't exist, and we properly error on it.
Resolves rdar://126169564