Add @_hasInitialValue to reliably mark variables with initialisers, and use it in TBDGen for StoredPropertyInitializer symbols #18385

huonw · 2018-07-31T05:49:18Z

The information about whether a variable/property is initialized is lost in the
public interface, but is, unfortunately, required because it results in a symbol
for the initializer (if a class/struct init is inlined, it will call
initializers for properties that it doesn't initialize itself). This is
important to preserve for TBD file generation.

Using an attribute rather than just a bit on the VarDecl means this fits into
the scheme for module interfaces: textual/valid Swift.

To validate that this is (hopefully) the only case like this, it also adds a test that is the union of all the other TBD tests, meaning multiple interesting files.

huonw · 2018-07-31T05:49:30Z

@swift-ci please smoke test

huonw · 2018-07-31T09:04:39Z

(Note to self: Linux failure implies need to add // REQUIRES: objc_interop)

I think I'll change the name of this to @_hasInitializer.

slavapestov · 2018-07-31T09:18:57Z

lib/AST/Decl.cpp

  auto F = InitAndFlags.getInt();
  if (E) {
    InitAndFlags.setInt(F - Flags::Removed);
    InitAndFlags.setPointer(E);
+    if (!PreviousInit) {


Is there no better place for this, maybe in Sema?

These initialisers get set and unset all over the place. Are you thinking that the attribute should be added once, after all of that?

It does seem like there's a lot of variables created and not in several places throughout the compiler and REPL, etc., and it seems not all of them go through the "obvious" type checker paths (e.g. property behavior's completePropertyBehaviorStorage sets everything up perfectly, and skips the main checker). It seems unfortunate to push this detail to be repeated at all such code paths (possibly including in lldb), given it is entirely computable from info given to this call, but I agree that it's a little ugly to have it here.

Variables created in the REPL never require TBD generation. Same for LLDB. Property behaviors are obsolete and probably getting removed at some point. I don't think any of the derived conformances etc in the type checker have initial values either. Please add this attribute when checking the initial value expression in the type checker.

I've moved it to TypeChecker::typeCheckPatternBinding.

slavapestov · 2018-07-31T09:19:42Z

test/IDE/print_ast_tc_decls.swift

@@ -108,7 +108,7 @@ struct d0100_FooStruct {
 // PASS_COMMON-LABEL: {{^}}struct d0100_FooStruct {{{$}}

  var instanceVar1: Int = 0
-// PASS_COMMON-NEXT: {{^}}  var instanceVar1: Int{{$}}
+// PASS_COMMON-NEXT: {{^}}  @_bindingHasInit var instanceVar1: Int{{$}}


Can you change swift-ide-test to not print this attribute, since it will confuse users who see it in generated interfaces in Xcode?

SourceKit generally ignores it (e.g. it isn't included in the lists of attributes in its RPC protocol thing), but generated interfaces go through a different route?

UserInaccessible takes care of this. swift-ide-test prints all attributes by default, IIRC.

jrose-apple · 2018-07-31T16:10:20Z

Hm. The information is derivable from whether there are any inlinable initializers, right? Though I guess that's not just a scan of the main declaration for a struct, since they might be declared in extensions.

huonw · 2018-08-01T23:15:45Z

The information is derivable from whether there are any inlinable initializers, right?

I don't think so, we'd also have to look at the body of those initialisers to see how they initialise the properties, e.g.:

struct NoSymbol {
    var x: Int
    @inlineable init() { x = 10 }
}

struct Symbol {
    var x: Int = 10
    @inlineable init() {}
}

Additionally, the current logic has a symbol for the property initialisers whether or not there is an @inlineable init. Maybe we should change this?

jrose-apple · 2018-08-02T00:01:30Z

There are few things going on here:

In a non-resilient library, the property initializers always have to be public, because until SE-0189 anyone could add their own non-delegating initializer. It's unclear whether we'll be able to break that in later releases given that it was supported in Swift 4.0 and thus only produces a warning under -swift-version 4 and -swift-version 4.2.
With a resilient library, SE-0189 is always enforced, so you only need to worry about initializers within the module. You obviously don't need to worry about non-inlinable initializers, which includes all non-fixed-contents structs. So the choice of whether or not to give the initial values public symbols is controlled by whether the struct is fixed-contents; it could check for inlinable initializers instead. (@slavapestov, I may have gotten some details of this wrong. Does it make the initial value expressions public or shared-linkage serialized?)

huonw · 2018-08-02T14:03:02Z

In a non-resilient library, the property initializers always have to be public, because until SE-0189 anyone could add their own non-delegating initializer. It's unclear whether we'll be able to break that in later releases given that it was supported in Swift 4.0 and thus only produces a warning under -swift-version 4 and -swift-version 4.2.

Ah, okay. It seems this case contradicts your previous point, as it won't be covered by scanning a module for @inlineable init. Additionally, even in an always-SE-0189 world, we won't want to be emitting public symbols for variables that don't have public initializers meaning it seems like we need a patch like this one, in some form.

I think I might be missing something?

jrose-apple · 2018-08-02T16:13:22Z

Now that I think about it, I suspect we don't want to add public symbols for initial value expressions in a resilient build at all. That would mean adding or removing one would be a breaking change. That in turn suggests serializing them along with the initializer.

In the non-resilient case, I think you're right, I got it wrong the first time. We always need these symbols.

huonw · 2018-08-02T22:33:34Z

That in turn suggests serializing them along with the initializer.

This means we need to do a @usableFromInlineable check for everything the initialiser uses. In a (hypothetical) world where everything is explicit, it seems we should even require people to mark the initialiser itself @inlinable/@usableFromInlineable, since changes to it won't be reflected downstream?

slavapestov · 2018-08-02T23:20:25Z

Current behavior:

Resilient module, fixed layout struct: We check that initial values only reference public and @usableFromInline symbols. The initial value stub is itself inlinable, and not public.
Resilient module, resilient struct: You can never inline a designated initializer so the initial value stub is not public and no restrictions are placed on it.
Non-resilient module: This is the case where the initial value stub is public, because we cannot enforce restrictions on initial value expressions (source compatibility) nor can we can ban inlinable designated inits.

Note that we already ban designated inits from being added from outside the module, but those cannot reference initial value expressions anyway -- since we have no cross-module knowledge they exist. The only way to reference an initial value expression is to inline an inlinable designated init from outside the module, and the only time the initial value expression creates a public symbol is with a non-resilient build.

jrose-apple · 2018-08-02T23:50:09Z

That's also a little scary, since it means that a let can have a different value from a cross-module initializer if they ignore the warning in Swift 4 mode. But I guess "you were warned".

slavapestov · 2018-08-03T00:22:33Z

Also how about @_hasInitialValue for a name? I don't like how we use the term 'property initializer' for these because its too similar to 'initializer' meaning 'constructor' (another unfortunate bit of overloading).

slavapestov · 2018-08-03T01:38:45Z

@jrose-apple Nice, I didn't even realize that thing about the 'let'. Good thing we banned this, then. :) Note that it does do the right thing in the cross-file case in non-WMO mode, and it doesn't even have to re-typecheck the expression unless its also used to infer the type.

jrose-apple · 2018-08-03T02:29:13Z

~~I still don't think we need an attribute at all. It sounds like it's fully derivable from the description we have above. None of that requires looking at initializer bodies.~~ Ah, I see. Without this, we'd have to assume every member might have an initial value when doing a non-whole-module build. Okay.

(And yes, I'm also in favor of "initial value" since "initializer" is taken.)

huonw · 2018-08-03T02:39:20Z

I don't understand what you mean by "fully derivable"? The problem this patch is solving is when generating a TBD file by merging modules, or by looking at the textual interface, there's absolutely no record of an initial value existing. I assume we don't want to have symbols for an initialiser for every property "just in case", that is, we only want symbols for ones with actual initial values.

huonw · 2018-08-03T12:48:24Z

@swift-ci please smoke test

jrose-apple

Okay, I think this looks pretty good, then, at least for non-resilient libraries. Sorry for misunderstanding the problem and going off in the wrong direction.

jrose-apple · 2018-08-03T17:48:38Z

include/swift/AST/Attr.def

@@ -376,6 +376,10 @@ SIMPLE_DECL_ATTR(_forbidSerializingReference, ForbidSerializingReference,
  OnAnyDecl |
  LongAttribute | RejectByParser | UserInaccessible | NotSerialized,
  77)
+SIMPLE_DECL_ATTR(_hasInitialValue, HasInitialValue,
+  OnVar |
+  SILOnly | UserInaccessible,


Oops, I think UserInaccessible is fine here. No need to limit it to SIL mode; no one's actually going to try to type it.

jrose-apple · 2018-08-03T17:50:10Z

lib/AST/ASTVerifier.cpp

@@ -250,6 +250,16 @@ class Verifier : public ASTWalker {
    GenericEnv.push_back({DC});
  }

+  /// \brief Returns the SourceFile for this AST, if it was parsed from a .swift
+  /// or (if \c allowSIL) .sil file.


Why this change? That is, what condition doesn't hold for this new attribute?

jrose-apple · 2018-08-03T17:50:36Z

lib/Sema/CodeSynthesis.cpp

@@ -1324,7 +1325,7 @@ void TypeChecker::completePropertyBehaviorStorage(VarDecl *VD,
  Storage->setImplicit();
  Storage->setAccess(AccessLevel::Private);
  Storage->setSetterAccess(AccessLevel::Private);
-  
+


Sorry, this file only has whitespace changes now; can you remove them?

jrose-apple · 2018-08-03T17:51:05Z

include/swift/AST/Decl.h

+      : ThePattern(P), EqualLoc(EqualLoc), InitAndFlags(nullptr, {}),
+        InitContext(InitContext) {
+    // Setting the initializer does some book-keeping, so we do that separately.
+    setInit(E);


This doesn't seem to be true anymore. Maybe it can go back to the way it was?

Ah, yeah. This and the change in ASTVerifier and CodeSynthesis are all just left overs from previous versions. Reverted.

jrose-apple · 2018-08-03T17:52:57Z

lib/TBDGen/TBDGen.cpp

+    entry.getPattern()->forEachVariable([&](VarDecl *VD) {
+      // Non-global variables might have an explicit initializer symbol.
+      if (VD->getAttrs().hasAttribute<HasInitialValueAttr>() &&
+          !isGlobalOrStaticVar(VD)) {


Oh no, I forgot that this is also relevant for classes. *sigh* Seems reasonable. Is it worth checking to see if this is a resilient module, though? (ModuleDecl::getResilienceStrategy)

Yes, might as well do that change here.

huonw · 2018-08-06T00:56:53Z

@swift-ci please smoke test

huonw · 2018-08-06T02:12:25Z

@swift-ci please smoke test

…bles.

The information about whether a variable/property is initialized is lost in the public interface, but is, unfortunately, required because it results in a symbol for the initializer (if a class/struct `init` is inlined, it will call initializers for properties that it doesn't initialize itself). This is important to preserve for TBD file generation. Using an attribute rather than just a bit on the VarDecl means this fits into the scheme for module interfaces: textual/valid Swift.

…alizer expression. The initializer expression is lost in the public interface (in Swift modules and the textual interface), but the attribute is preserved.

This checks that all combinations of optimized & non-optimized, and whole-module optimization & incremental compilation give the same result, on a module where this is actually interesting (i.e. has multiple files so the behaviour differs between the two).

huonw · 2018-08-06T23:56:05Z

@swift-ci please smoke test

slavapestov · 2018-08-07T08:44:30Z

LGTM

huonw requested a review from jrose-apple July 31, 2018 05:49

slavapestov reviewed Jul 31, 2018

View reviewed changes

huonw changed the title ~~Add @_bindingHasInit to reliable mark variables with initialisers, and use it in TBDGen for StoredPropertyInitializer symbols~~ Add @_hasInitialValue to reliably mark variables with initialisers, and use it in TBDGen for StoredPropertyInitializer symbols Aug 3, 2018

huonw force-pushed the tbd-property-initializer branch from e6cb7ac to 8f60651 Compare August 3, 2018 12:09

jrose-apple reviewed Aug 3, 2018

View reviewed changes

huonw force-pushed the tbd-property-initializer branch from 8f60651 to 7c3c5ee Compare August 6, 2018 00:52

huonw force-pushed the tbd-property-initializer branch from 7c3c5ee to c26b31d Compare August 6, 2018 02:09

huonw added 4 commits August 7, 2018 09:30

[TBDGen] Don't list symbols for non-public static/global stored varia…

1c622fd

…bles.

[TBDGen] Look for @_hasInitialValue on variables rather than an initi…

e57b332

…alizer expression. The initializer expression is lost in the public interface (in Swift modules and the textual interface), but the attribute is preserved.

huonw force-pushed the tbd-property-initializer branch from c26b31d to e952579 Compare August 6, 2018 23:55

huonw merged commit bfa4c73 into swiftlang:master Aug 7, 2018

huonw deleted the tbd-property-initializer branch August 7, 2018 03:13

Add @_hasInitialValue to reliably mark variables with initialisers, and use it in TBDGen for StoredPropertyInitializer symbols #18385

Add @_hasInitialValue to reliably mark variables with initialisers, and use it in TBDGen for StoredPropertyInitializer symbols #18385

Uh oh!

Conversation

huonw commented Jul 31, 2018

Uh oh!

huonw commented Jul 31, 2018

Uh oh!

huonw commented Jul 31, 2018

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!

slavapestov Aug 2, 2018 • 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!

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!

jrose-apple commented Jul 31, 2018

Uh oh!

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

Uh oh!

Uh oh!

jrose-apple commented Aug 2, 2018

Uh oh!

huonw commented Aug 2, 2018

Uh oh!

jrose-apple commented Aug 2, 2018

Uh oh!

huonw commented Aug 2, 2018

Uh oh!

slavapestov commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Aug 2, 2018

Uh oh!

slavapestov commented Aug 3, 2018

Uh oh!

slavapestov commented Aug 3, 2018

Uh oh!

jrose-apple commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huonw commented Aug 3, 2018

Uh oh!

huonw commented Aug 3, 2018

Uh oh!

jrose-apple left a comment

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!

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!

huonw commented Aug 6, 2018

Uh oh!

huonw commented Aug 6, 2018

Uh oh!

slavapestov Aug 2, 2018 •

edited

Loading

huonw commented Aug 1, 2018 •

edited

Loading

slavapestov commented Aug 2, 2018 •

edited

Loading

jrose-apple commented Aug 3, 2018 •

edited

Loading