[Concurrency] Handle more expr types for inout checking #35325

etcwilde · 2021-01-08T23:52:52Z

This patch fixes a compiler crash when optional values were being unwrapped and passed to a function. The crash was caused because diagnoseInOutArg could only handle LookupExpr and DeclRefExpr. This patch adds code to unwrap more kinds of expressions.

I've introduced a new static function, getInoutTopExpr , which walks through the expression types it knows about down to what is hopefully either a LookupExpr or a DeclRefExpr, from which we can pull the correct ValueDecl.
If we hit an InOutExpr sub-expression while traversing, we just return a nullptr and jump out of the diagnostic. If we don't do this, we will get duplicate error messages emitted since it appears to get re-checked. The duplicate error messages show up on await modifyAsynchronously(&points[0].x) and await modifyAsynchronously(&points[0].z!).

Fixes: rdar://72906215

DougGregor

This is definitely progress, and I appreciate the greater test coverage. I'm a bit nervous that we haven't handled all of the cases, but I don't have a good sense of how to ensure that we have handled all of the cases here, so +1 for progress.

DougGregor · 2021-01-09T00:00:12Z

lib/Sema/TypeCheckConcurrency.cpp

+    if (const InOutExpr *inout = dyn_cast<InOutExpr>(currentExpr)) {
+      return nullptr; // The AST walker will hit this again
+    } else if (const LookupExpr *baseArg = dyn_cast<LookupExpr>(currentExpr)) {
+      while (const LookupExpr *nextLayer = dyn_cast<LookupExpr>(baseArg->getBase()))


I don't think we need this loop, given that you'll go around the outer loop again anyway?

etcwilde · 2021-01-09T01:26:55Z

@swift-ci smoke test please

etcwilde · 2021-01-09T01:28:08Z

@swift-ci please smoke test

etcwilde · 2021-01-09T01:30:31Z

@swift-ci please smoke test windows

airspeedswift · 2021-01-09T17:00:13Z

@swift-ci please smoke test windows platform

kavon

Like Doug, I do wonder what other expression forms might be missing, but this is good as-is.

What happens with other kinds of operators, e.g., function application, unary negation, addition, and the conditional ternary operator ? :? Can they be used inside of an in-out argument expression? (I don't know the normal restrictions on in-out)

etcwilde · 2021-01-11T20:49:50Z

Like Doug, I do wonder what other expression forms might be missing, but this is good as-is.

What happens with other kinds of operators, e.g., function application, unary negation, addition, and the conditional ternary operator ? :? Can they be used inside of an in-out argument expression? (I don't know the normal restrictions on in-out)

I agree on the issue with other expressions. I've tested what I know how to hit and can be passed inout, which is certainly not exhaustive. Ternary operators only return non-mutable results, so it can't be passed inout.

I've kind of started going through the list of Expr types that inherit directly from Expr and handling those, where I know how to get a test case.

From that I'm missing the following expression types (as defined in Expr.h):

ErrorExpr -- Maybe we should propagate that up instead of exploding. Would probably be good.
CodeCompletionExpr -- no idea how to get one of these injected into an AST
LiteralExpr -- Literals are not mutable
TapExpr -- Not entirely sure what this is, or how it would get injected into the AST as a paramter
DiscardAssignmentExpr -- Can only appear on left side of an assignment
SuperRefExpr -- Not sure how you'd get one of these as part of an inout parameter
TypeExpr -- Not sure how to get one of these as part of an inout parameter
OtherConstructorDeclRefExpr -- Not sure how it would be a part of a valid inout parameter
OverloadSetRefExpr -- ??? Not sure what this is.
UnresolvedDeclRefExpr -- Not sure how to get one in Swift, though maybe it's possible?
UnresolvedMemberExpr -- Not sure how to get one in swift, though maybe it's possible?
AnyTryExpr -- Results from a function aren't mutable, I'm not sure how else you might use try.
IdentityExpr -- ??? Not sure what this is. self is immutable passed directly. Might want to handle ParenExprs like in myFunc(&(foo)) or AwaitExpr, though is the result of an AwaitExpr mutable?
TupleExpr -- TupleExpr's aren't mutable myFunc(&(1,2,3)) isn't legal.
CollectionExpr -- ??? Seems like it's either a ParenExpr or TupleExpr, but not totally clear.
KeyPathApplicationExpr -- ??? Not sure what this is.
UnresolvedDotExpr -- ??? Need to figure out how to get unresolved types to test this
TupleElementExpr -- This should only be a sub-expression of a TupleExpr, which is already illegal.
OptionalEvaluaionExpr -- Something like foo.bar?(), which would not be mutable since it's the result of a function call
MakeTemporarilyEscapableExpr -- ??? Not sure.
OpenExistentialExpr -- ???
UnresolvedSpcializeExpr -- ???
VarargExpansionExpr -- Comment says it's not surfaced. Probably don't need?
SequenceExpr -- ???
AbstractClosureExpr -- I don't think we can pass one of these inout?
CaptureListExpr -- Pretty sure this can't be passed inout
DynamicTypeExpr -- Pretty sure this can't be passed inout.
OpaqueValueRef -- I'm not sure how to get one of these in the AST, but we might want to handle passing it?
PropertyWrapperValuePlaceholderExpr -- ??? I'm pretty sure this can't appear as part of the inout parameter subtree.
DefaultArgumentExpr -- Also probably can't be part of an inout parameter subtree.
ApplyExpr -- Can't be pass inout since the result of an application is not mutable
DotSyntaxBaseIgnoredExpr -- ???
ExplicitCastExpr -- Result is not mutable, can't be passed inout.
ArrowExpr -- Goes on function decls, not sure how it would be passed as part of a parameter
RebindSelfInConstructorExpr -- ??? Not sure what this is for
IfExpr -- ternary and friends are not mutable
EnumIsCaseExpr -- ?? Not sure what this is, but It sounds like it would not be mutable.
AssignExpr -- Immutable
UnresolvedPatternExpr -- ??? Not sure how to get one of these
EditorPlaceholderExpr -- Not sure how to get one of these
LazyInitializerExpr -- Not sure how to get one of these
ObjcSelectorExpr -- I'm not familiar with this one? Can it return stuff?
KeyPathExpr -- Not familiar with this one. Looks like the return value is immutable.
KeyPathDotExpr -- ??
OneWayExpr -- ??

I definitely need to add a clause for ParenExpr, but otherwise I haven't seen the other expression types type-check far enough to cause the crash. If we wanted to weaken the check, I could make the else-clause return a nullptr which would skip out early on unknown expression types. I'm hesitant to do this since we could potentially accept buggy code for one version, then add the missing clause to the check and make a source-breaking change. Thoughts?

etcwilde · 2021-01-11T21:37:02Z

@swift-ci please smoke test

etcwilde · 2021-01-12T00:14:35Z

@swift-ci please smoke test

kavon

Some feedback to help improve the implementation:

kavon · 2021-01-16T00:16:44Z

lib/Sema/TypeCheckConcurrency.cpp

+///
+/// The return will either be a LookupExpr or a DeclRefExpr.
+/// If part of the sub-expression isn't interesting, returns nullptr
+static const Expr *getInoutTopExpr(const InOutExpr *arg) {


I wonder if, instead of trying to manually enumerate all of the possible expressions that could appear, you could try using some existing infrastructure that can handle some or all of those cases for you in this search for a DeclRef.

For example, we have Expr::getReferencedDecl which appears to do something similar to your getInoutTopExpr function. A key thing to note is that getReferencedDecl would return the referenced member, and not the base, of a LookupExpr (see the cases for MemberRef, as an example). I don't think that difference would limit you from using it in some capacity, though. (for example, you could determine the base Decl from the member Decl returned, or if that doesn't work, use both getReferencedDecl and your own search loop)

There's also Expr::forEachChildExpr which one could use to do a generic walk over the whole expression tree though perhaps that's a bit much?

Okay, I've switched it to using expr::forEachChildExpr. Still a little nervous that it might accept invalid code.
Not a huge fan of the double-lambdas, but it should do the right things.

etcwilde · 2021-01-22T00:41:03Z

@swift-ci please smoke test

kavon

I think this version is quite clean now. 👍🏼

We were handling weren't handling all of the expression types that can show up as part of a parameter. As a result, the compiler was crashing on correct code. This patch introduces handling for far more expression types and the appropriate tests for each. Added handling includes: - Gentle optional unwrap - Forceful optional unwrap - Identity expressions (await, parens, etc) - Implicit conversion exprs

etcwilde · 2021-01-26T22:46:07Z

Fixing merge conflicts, then merging. Thanks for the reviews.

Replacing ad-hoc expression tree with forEachChild traversal and matching the desired expression. This should avoid crashes, but may accept invalid code in the future.

etcwilde · 2021-01-26T22:51:07Z

@swift-ci please test

etcwilde requested review from kavon and DougGregor January 8, 2021 23:52

DougGregor approved these changes Jan 9, 2021

View reviewed changes

etcwilde requested review from phausler and DougGregor January 9, 2021 00:02

kavon reviewed Jan 11, 2021

View reviewed changes

etcwilde force-pushed the ewilde/handle-more-exprs-for-inout-checking branch from 2fa34d6 to 5d59261 Compare January 11, 2021 21:34

etcwilde force-pushed the ewilde/handle-more-exprs-for-inout-checking branch from 5d59261 to cb0fb34 Compare January 11, 2021 23:32

etcwilde requested a review from kavon January 14, 2021 23:59

kavon reviewed Jan 16, 2021

View reviewed changes

etcwilde requested a review from kavon January 22, 2021 00:44

kavon approved these changes Jan 26, 2021

View reviewed changes

Use forEachChild for traversal instead of ad-hoc traversal

3a3aaf1

Replacing ad-hoc expression tree with forEachChild traversal and matching the desired expression. This should avoid crashes, but may accept invalid code in the future.

etcwilde force-pushed the ewilde/handle-more-exprs-for-inout-checking branch from 447ff4a to 3a3aaf1 Compare January 26, 2021 22:49

etcwilde merged commit 3efa18f into swiftlang:main Jan 27, 2021

etcwilde deleted the ewilde/handle-more-exprs-for-inout-checking branch January 27, 2021 03:37

[Concurrency] Handle more expr types for inout checking #35325

[Concurrency] Handle more expr types for inout checking #35325

Uh oh!

Conversation

etcwilde commented Jan 8, 2021

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

DougGregor Jan 9, 2021

Choose a reason for hiding this comment

Uh oh!

etcwilde Jan 9, 2021

Choose a reason for hiding this comment

Uh oh!

etcwilde commented Jan 9, 2021

Uh oh!

etcwilde commented Jan 9, 2021

Uh oh!

etcwilde commented Jan 9, 2021

Uh oh!

airspeedswift commented Jan 9, 2021

Uh oh!

kavon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etcwilde commented Jan 11, 2021

Uh oh!

etcwilde commented Jan 11, 2021

Uh oh!

etcwilde commented Jan 12, 2021

Uh oh!

kavon left a comment

Choose a reason for hiding this comment

Uh oh!

kavon Jan 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etcwilde Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

etcwilde commented Jan 22, 2021

Uh oh!

kavon left a comment

Choose a reason for hiding this comment

Uh oh!

etcwilde commented Jan 26, 2021

Uh oh!

etcwilde commented Jan 26, 2021

Uh oh!

Uh oh!

kavon left a comment •

edited

Loading

kavon Jan 16, 2021 •

edited

Loading