Eliminate Observable circular reference errors via lazier TypeRefinementContext building #67642

DougGregor · 2023-08-01T05:44:54Z

Eager expansion of type refinement contexts (TRCs) for variables within pattern binding declarations is causing cyclic references in some places involving macros. Make this expansion lazy, triggered by walking into these pattern binding declarations as part of (e.g.) availability queries. Also be careful to avoid "has property wrapper" queries as part of building the type refinement context, because they tend to cause circularities.

Fixes rdar://112079160, a case where we would see circular references when extending an @Observable class from another source file.

DougGregor · 2023-08-01T05:45:42Z

@swift-ci please smoke test

DougGregor · 2023-08-01T05:45:48Z

@swift-ci please test source compatibility

DougGregor · 2023-08-01T06:17:11Z

@swift-ci please smoke test Linux

DougGregor · 2023-08-01T16:59:11Z

@swift-ci please smoke test

DougGregor · 2023-08-01T21:12:15Z

@swift-ci please smoke test

DougGregor · 2023-08-01T21:12:26Z

@swift-ci please test source compatibility

tshortli · 2023-08-01T22:21:47Z

lib/Sema/TypeCheckAvailability.cpp

+    // the property wrapper attribute and use its source range to create a
+    // TRC for the initializer expression.
+    //
+    // FIXME: Since we don't have an expression here, we can't build out its


Is this comment no longer true, since we can now lazily expand the TRC for the init expr? I'm having a little trouble following whether this is handled now, but it sounds like it might be from a comment in the request implementation.

I don't know that this has fully addressed the issue, because it seems like we'd need to add the TRCs for the property wrapper attributes (below). I'd need to dig into rdar://77841331 to see what it would take to fix that.

Aside from the bugs I've added, I suspect this is related... this fails to type-check both before and after my PR:

@available(macOS 10.15, *) func f() -> Int { 17 } class X { @available(macOS 10.14, *) private static let value: Int = { if #available(macOS 10.15, *) { return f() // produces an error here saying that f() is not available } return 0 }() }

Ok, thanks for checking. I'll take a look at fixing that eventually!

I changed the approach a little bit to in this commit deal with a failure in the source compatibility suite, which also fixes the example I gave above---an existing bug that's now fixed by my PR. It doesn't address rdar://77841331, though

DougGregor · 2023-08-01T22:31:25Z

@swift-ci please smoke test Linux

DougGregor · 2023-08-02T17:40:30Z

@swift-ci please smoke test

DougGregor · 2023-08-02T17:40:36Z

@swift-ci please test source compatibility

DougGregor · 2023-08-02T17:43:28Z

@swift-ci please smoke test macOS

DougGregor · 2023-08-02T17:43:33Z

@swift-ci please smoke test Windows

DougGregor · 2023-08-02T17:43:41Z

@swift-ci please test source compatibility release

DougGregor · 2023-08-02T17:51:05Z

@swift-ci please smoke test macOS

DougGregor · 2023-08-02T17:51:12Z

@swift-ci please smoke test Windows

DougGregor · 2023-08-02T17:51:27Z

@swift-ci please test source compatibility release

DougGregor · 2023-08-02T18:57:38Z

@swift-ci please test source compatibility release

DougGregor · 2023-08-02T19:00:53Z

@swift-ci please smoke test Windows

…e info Querying property wrappers involves semantic analysis that can cause cyclic references while building the type refinement context, and it's unnecessary: we need only know that these are custom attributes to incorporate their source ranges. Switch to the simpler/cheaper query. A small part of fixing the cyclic references in rdar://112079160.

Eager expansion of type refinement contexts (TRCs) for variables within pattern binding declarations is causing cyclic references in some places involving macros. Make this expansion lazy, triggered by walking into these pattern binding declarations as part of (e.g.) availability queries. Another step toward fixing the cyclic references in rdar://112079160.

… lazily The check for "has property wrappers" as part of determining whether the layout of a variable is exposed to clients can trigger reference cycles. Push this check later, which eliminates these cycles for types that aren't frozen/fixed-layout. This is a hack, not a real fix, but it eliminates the cyclic references observed in rdar://112079160.

…anges

Add a test case for Observable types that are extended from other source files. Prior to the recent changes to make `TypeRefinementContext` more lazy, this would trigger circular references through the `TypeRefinementContextBuilder`. Finishes rdar://112079160.

…lder This state is a holdover from when accessors we stored "alongside" their variable declarations, rather than contained within them. That's no longer the case, so we don't need to track this information any more.

The type refinement context builder had a bunch of logic to try to model type refinement contexts for the first variable declaration that shows up within a pattern binding declaration. Instead, model this more syntactically by creating a type refinement context for the pattern binding declaration itself. This both addresses a regression in the handling of `if #available` within a closure that's part of an initializer, and fixes a bug in the same area where similar code has explicit availability annotations.

DougGregor · 2023-08-02T22:10:44Z

@swift-ci please smoke test

tshortli · 2023-08-02T23:29:16Z

lib/AST/Availability.cpp

@@ -749,3 +751,31 @@ ASTContext::getSwift5PlusAvailability(llvm::VersionTuple swiftVersion) {
 bool ASTContext::supportsVersionedAvailability() const {
  return minimumAvailableOSVersionForTriple(LangOpts.Target).has_value();
 }
+
+const Decl *
+swift::abstractSyntaxDeclForAvailableAttribute(const Decl *ConcreteSyntaxDecl) {


Interesting, it feels like there's some overlap here with the parentDeclForInferredAvailability function that I introduced. I don't know that the two can merge, but maybe parentDeclForInferredAvailability should call this?

tshortli · 2023-08-02T23:33:57Z

test/Sema/availability_versions.swift

+func f() -> Int { 17 }
+
+class StoredPropertiesWithAvailabilityInClosures {
+  private static let value: Int = {


This could maybe use a test case asserting that we diagnose a reference to f when there is no if #available.

Oh, we actually don't diagnose a reference to f there because it's an initializer, and those aren't exposed to the client (so they're checked at the deployment target)

So then should f have availability later than the deployment target? Otherwise, we're not necessarily testing that TRCs get built out properly for the body.

I have some test cases locally to check this, and will push them to a separate PR shortly.

test/stdlib/Observation/ObservableAvailabilityCycle.swift

slavapestov · 2023-08-03T14:43:01Z

lib/AST/Decl.cpp


  auto nominalAccess =
    parent->getFormalAccessScope(/*useDC=*/nullptr,
                                 /*treatUsableFromInlineAsPublic=*/true);
  if (!nominalAccess.isPublic()) return false;

-  return (parent->getAttrs().hasAttribute<FrozenAttr>() ||
-          parent->getAttrs().hasAttribute<FixedLayoutAttr>());
+  if (!parent->getAttrs().hasAttribute<FrozenAttr>() &&


Is it worth adding a comment here pointing out that we want to check the "easy" conditions before possibly evaluating requests?

compnerd · 2023-08-04T12:41:28Z

This seems to be causing some trouble on CI for macOS:

/Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/stdlib/Observation/Inputs/ObservableClass.swift:5:2: error: 'Observable()' is only available in macOS 14.0 or newer
@observable final public class ObservableClass {
^

slavapestov · 2023-08-04T14:18:08Z

Instead of reverting why not just disable the new test?

slavapestov · 2023-08-04T14:21:41Z

#67737 disables the new test. I filed rdar://113395709 for Doug

DougGregor requested review from tshortli, hborla, slavapestov and xedin as code owners August 1, 2023 05:44

DougGregor force-pushed the lazy-type-refinement-context branch from e659571 to 38b19c0 Compare August 1, 2023 16:59

tshortli reviewed Aug 1, 2023

View reviewed changes

DougGregor changed the title ~~Lazy type refinement context~~ Eliminate Observable circular reference errors via lazier TypeRefinementContext building Aug 1, 2023

tshortli approved these changes Aug 1, 2023

View reviewed changes

DougGregor added 5 commits August 2, 2023 15:07

[Type refinement context] Avoid creating implicit contexts with bad r…

0d779df

…anges

DougGregor added 2 commits August 2, 2023 15:07

DougGregor force-pushed the lazy-type-refinement-context branch from 51b7b4d to 37959de Compare August 2, 2023 22:10

DougGregor mentioned this pull request Aug 2, 2023

[5.9] Eliminate Observable circular reference errors via lazier TypeRefinementContext building #67689

Merged

tshortli reviewed Aug 2, 2023

View reviewed changes

tshortli approved these changes Aug 2, 2023

View reviewed changes

slavapestov reviewed Aug 3, 2023

View reviewed changes

DougGregor merged commit 300d04a into swiftlang:main Aug 3, 2023

DougGregor deleted the lazy-type-refinement-context branch August 3, 2023 19:47

compnerd mentioned this pull request Aug 4, 2023

Revert "Eliminate Observable circular reference errors via lazier TypeRefinementContext building" #67734

Closed

DougGregor mentioned this pull request Aug 4, 2023

Expand type refinement contexts to encompass attributes #67732

Merged

Eliminate Observable circular reference errors via lazier TypeRefinementContext building #67642

Eliminate Observable circular reference errors via lazier TypeRefinementContext building #67642

Uh oh!

Conversation

DougGregor commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DougGregor commented Aug 1, 2023

Uh oh!

DougGregor commented Aug 1, 2023

Uh oh!

DougGregor commented Aug 1, 2023

Uh oh!

DougGregor commented Aug 1, 2023

Uh oh!

DougGregor commented Aug 1, 2023

Uh oh!

DougGregor commented Aug 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DougGregor commented Aug 1, 2023

Uh oh!

DougGregor commented Aug 2, 2023

Uh oh!

DougGregor commented Aug 2, 2023

Uh oh!

DougGregor commented Aug 2, 2023

Uh oh!

DougGregor commented Aug 2, 2023

Uh oh!

DougGregor commented Aug 2, 2023

Uh oh!

DougGregor commented Aug 2, 2023

Uh oh!

DougGregor commented Aug 2, 2023

Uh oh!

DougGregor commented Aug 2, 2023

Uh oh!

DougGregor commented Aug 2, 2023

Uh oh!

DougGregor commented Aug 2, 2023

Uh oh!

DougGregor commented Aug 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd commented Aug 4, 2023

Uh oh!

slavapestov commented Aug 4, 2023

Uh oh!

slavapestov commented Aug 4, 2023

Uh oh!

Uh oh!

DougGregor commented Aug 1, 2023 •

edited

Loading