[Function builders] Use buildLimitedAvailability() for #available block #32994

DougGregor · 2020-07-20T04:24:58Z

The use of "if #available" in function builders can subvert availability
checking if the function builder carries all type information for the
values within the "then" block outside of the "else" block. Tighten up
the model in two ways:

Check whether the type coming out of an "if #available" references
any declarations that are not available in the outer context, to close
up the model.
If the function builder provides a buildLimitedAvailability(_:)
operation, call that on the result of the "then" block in an "if
that it cannot leak out of the "if #available"; if it doesn't, the
check above will still fire.

Stage this in with a warning so function builders out there in the wild
can adapt. We'll upgrade the warning to an error later.

Fixes rdar://problem/65021017.

The use of "if #available" in function builders can subvert availability checking if the function builder carries all type information for the values within the "then" block outside of the "else" block. Tighten up the model in two ways: * Check whether the type coming out of an "if #available" references any declarations that are not available in the outer context, to close up the model. * If the function builder provides a buildLimitedAvailability(_:) operation, call that on the result of the "then" block in an "if that it cannot leak out of the "if #available"; if it doesn't, the check above will still fire. Stage this in with a warning so function builders out there in the wild can adapt. We'll upgrade the warning to an error later. Fixes rdar://problem/65021017.

DougGregor · 2020-07-20T04:25:07Z

@swift-ci test

DougGregor · 2020-07-20T04:25:13Z

@swift-ci test source compatibility

theblixguy

I just have some very minor suggestions

theblixguy · 2020-07-20T12:11:00Z

include/swift/AST/DiagnosticsSema.def

@@ -5127,6 +5127,10 @@ NOTE(function_builder_infer_add_return, none,
 NOTE(function_builder_infer_pick_specific, none,
     "apply function builder %0 (inferred from %select{protocol|dynamic replacement of}1 %2)",
     (Type, unsigned, DeclName))
+WARNING(function_builder_missing_limited_availability, none,
+        "function builder %0 does not implement `buildLimitedAvailability`; "


Suggested change

"function builder %0 does not implement `buildLimitedAvailability`; "

"function builder %0 does not implement 'buildLimitedAvailability'; "

It would also be great to put this in the YAML diagnostic localization file :)

theblixguy · 2020-07-20T12:13:22Z

lib/Sema/BuilderTransform.cpp

@@ -498,10 +517,20 @@ class BuilderClosureVisitor
    if (!cs || !thenVar || (elseChainVar && !*elseChainVar))
      return nullptr;

+    // If there is a #available in the condition, the 'then' will need to
+    // be wrapped in a call to buildAvailabilityErasure(_:), if available.


Suggested change

// be wrapped in a call to buildAvailabilityErasure(_:), if available.

// be wrapped in a call to buildLimitedAvailability(_:), if available.

theblixguy · 2020-07-20T12:14:52Z

lib/Sema/BuilderTransform.cpp

@@ -1175,6 +1204,35 @@ class BuilderClosureRewriter
            capturedThen.first, {capturedThen.second.front()}));
    ifStmt->setThenStmt(newThen);

+    // Look for a #available condition. If there is one, we need to check
+    // that the resulting type of the "then" does refer to any types that


Suggested change

// that the resulting type of the "then" does refer to any types that

// that the resulting type of the "then" doesn’t refer to any types that

theblixguy · 2020-07-20T12:15:15Z

lib/Sema/BuilderTransform.cpp

+    // that the resulting type of the "then" does refer to any types that
+    // are unavailable in the enclosing context.
+    //
+    // Note that this is for staging in support for


Nit: Incomplete sentence.

jckarter · 2020-07-20T16:34:52Z

Looks good to me!

For an added bit of paranoia, should we look at the availability of the buildLimitedAvailability implementation and propagate that too, or at least just throw up a warning saying that if buildLimitedAvailability is itself availability-constrained it defeats the purpose of the feature?

rjmccall

Approved for the short term.

rjmccall · 2020-07-20T17:45:41Z

lib/Sema/BuilderTransform.cpp

+    // that the resulting type of the "then" does refer to any types that
+    // are unavailable in the enclosing context.
+    //
+    // Note that this is for staging in support for


Incomplete sentence

rjmccall · 2020-07-20T17:47:40Z

lib/Sema/BuilderTransform.cpp

+                              nominal, loc, dc)) {
+          // Note that the problem is with the function builder, not the body.
+          // This is for staging only. We want to disable #available in
+          // function builders that don't support this operation.


This doesn't seem reasonable in general; function builders which don't propagate types structurally are probably fine. Can we not diagnose that an unavailable type has escaped the context?

We can, and you're right: I'll zap the comment when address the other comments in a follow-up PR. Later in 5.3, we'll turn this warning into an error.

DougGregor · 2020-07-21T06:29:57Z

For an added bit of paranoia, should we look at the availability of the buildLimitedAvailability implementation and propagate that too, or at least just throw up a warning saying that if buildLimitedAvailability is itself availability-constrained it defeats the purpose of the feature?

The call to buildLimitedAvailability will be inside the "then" block, so its availability will get checked there. The check for an unavailability type that escapes buildLimitedAvailability will catch anything that slips through.

DougGregor · 2020-07-22T16:38:29Z

Cleanup PR over at #33046

DougGregor requested review from jckarter and rjmccall July 20, 2020 04:24

theblixguy reviewed Jul 20, 2020

View reviewed changes

rjmccall approved these changes Jul 20, 2020

View reviewed changes

DougGregor merged commit cde899d into swiftlang:master Jul 21, 2020

DougGregor mentioned this pull request Jul 21, 2020

[5.3] [Function builders] Use buildLimitedAvailability() for #available block #32995

Merged

DougGregor deleted the function-builder-limited-availability branch July 21, 2020 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Function builders] Use buildLimitedAvailability() for #available block #32994

[Function builders] Use buildLimitedAvailability() for #available block #32994

Uh oh!

DougGregor commented Jul 20, 2020

Uh oh!

DougGregor commented Jul 20, 2020

Uh oh!

DougGregor commented Jul 20, 2020

Uh oh!

theblixguy left a comment

Uh oh!

theblixguy Jul 20, 2020

Uh oh!

theblixguy Jul 20, 2020

Uh oh!

theblixguy Jul 20, 2020

Uh oh!

theblixguy Jul 20, 2020 •

edited

Loading

Uh oh!

jckarter commented Jul 20, 2020

Uh oh!

rjmccall left a comment

Uh oh!

rjmccall Jul 20, 2020

Uh oh!

rjmccall Jul 20, 2020

Uh oh!

DougGregor Jul 21, 2020

Uh oh!

DougGregor commented Jul 21, 2020

Uh oh!

DougGregor commented Jul 22, 2020

Uh oh!

Uh oh!

	"function builder %0 does not implement `buildLimitedAvailability`; "
	"function builder %0 does not implement 'buildLimitedAvailability'; "

	// be wrapped in a call to buildAvailabilityErasure(_:), if available.
	// be wrapped in a call to buildLimitedAvailability(_:), if available.

	// that the resulting type of the "then" does refer to any types that
	// that the resulting type of the "then" doesn’t refer to any types that

[Function builders] Use buildLimitedAvailability() for #available block #32994

[Function builders] Use buildLimitedAvailability() for #available block #32994

Uh oh!

Conversation

DougGregor commented Jul 20, 2020

Uh oh!

DougGregor commented Jul 20, 2020

Uh oh!

DougGregor commented Jul 20, 2020

Uh oh!

theblixguy left a comment

Choose a reason for hiding this comment

Uh oh!

theblixguy Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

theblixguy Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

theblixguy Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

theblixguy Jul 20, 2020 • 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!

jckarter commented Jul 20, 2020

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

rjmccall Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

rjmccall Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

DougGregor Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

DougGregor commented Jul 21, 2020

Uh oh!

DougGregor commented Jul 22, 2020

Uh oh!

Uh oh!

theblixguy Jul 20, 2020 •

edited

Loading