Check that signatures of @usableFromInline declarations only reference @usableFromInline or public types #16586

slavapestov · 2018-05-14T01:11:05Z

We were performing these checks for references from the body of inlinable functions as part of availability checking, but we also have to do similar checks in access level checking for types that are referenced from the signature of the function itself.

Also, allow type aliases to be @usableFromInline, and ensure that only @usableFromInline type aliases are referenced from inlinable bodies.

This plugs some holes in the model that could cause linker errors on invalid code. Fixes rdar://problem/32722385, rdar://problem/39532028,

slavapestov · 2018-05-18T08:19:14Z

@swift-ci Please test

swift-ci · 2018-05-18T08:41:53Z

Build failed
Swift Test OS X Platform
Git Sha - a0c6ca0393c053e4a6061733da9db511141ad1fb

swift-ci · 2018-05-18T08:46:53Z

Build failed
Swift Test Linux Platform
Git Sha - a0c6ca0393c053e4a6061733da9db511141ad1fb

slavapestov · 2018-05-18T09:04:57Z

@swift-ci Please test

slavapestov · 2018-05-18T09:05:07Z

@swift-ci Please test

slavapestov · 2018-05-18T09:24:27Z

@swift-ci Please test source compatibility

slavapestov · 2018-05-18T09:24:44Z

@swift-ci Please test source compatibility

swift-ci · 2018-05-18T09:37:22Z

Build failed
Swift Test Linux Platform
Git Sha - 6f9dc2c3f278f2e434fa74ca4aeb7d3b6242b4ae

jrose-apple

Whew, that was more than I thought it would be! I do have some concerns about this implementation, though.

jrose-apple · 2018-05-18T16:19:25Z

include/swift/AST/DiagnosticsSema.def

@@ -1317,20 +1328,25 @@ ERROR(unsupported_nested_protocol,none,
 // Type aliases
 ERROR(type_alias_underlying_type_access,none,
      "type alias %select{must be declared %select{"
-      "%select{private|fileprivate|internal|%error|%error}2|private or fileprivate}3"
+      "%select{private|fileprivate|internal|%error|%error}1|private or fileprivate}3"


(here and below) This was correct the way it was before. Argument 1 is the current access; argument 2 is the required access.

Are you sure? When isExplicit is true, %1 is the decl's getFormalAccess(), when isExplicit is false, %1 is the AccessScope's requiredAccessForDiagnostics(). On the other hand, %2 is the AccessScope's accessLevelForDiagnostics().

I noticed some inconsistency here and cleaned it up. Some diagnostics were using %2 here for the isExplicit == false case, but this appeared to generate a wrong diagnostic because of the distinction between requiredAccessForDiagnostics() and accessLevelForDiagnostics() with private vs fileprivate. This change and tests for it was in a separate patch so it should be easier to review that way.

Note the difference in messages: "must be declared X" vs. "cannot be declared Y". There might be mistakes at some of the use sites, but they're definitely not supposed to be the same value.

"When isExplicit is true, %1 is the decl's getFormalAccess(), when isExplicit is false, %1 is the AccessScope's requiredAccessForDiagnostics()" -- so they're different values :)

So using %1 vs %2 in the !isExplicit case is almost equivalent, except for private vs fileprivate.

I see: you changed the call site too. Please don't do this; it muddles the notion of what each parameter is used for. At that point I'd rather just use different diagnostics.

jrose-apple · 2018-05-18T16:22:19Z

lib/Sema/TypeCheckDecl.cpp

  TypeChecker::TypeAccessScopeCacheMap &Cache;

 protected:
  ASTContext &Context;
  Optional<AccessScope> Scope = AccessScope::getPublic();

  AccessScopeChecker(const DeclContext *useDC,
+                     bool treatUsableFromInlineAsPublic,


You're definitely starting to convince me that UsableFromInline is really a new access level.

jrose-apple · 2018-05-18T16:24:11Z

lib/Sema/TypeCheckDecl.cpp

                                    AccessScope accessScope,
                                    AccessLevel contextAccess) {
  if (!params)
    return;

+  if (checkUsableFromInline) {
+    if (auto *VD = dyn_cast<ValueDecl>(owner)) {
+      if (VD->getFormalAccess() != AccessLevel::Internal)


This is a suspicious condition—this could be above internal or it could be below internal.

checkUsableFromInline means only check @usableFromInline and nothing else. So we call this function once with checkUsableFromInline=false if the decl is not @usableFromInline; if it is, we call it twice, first with false then with true.

jrose-apple · 2018-05-18T16:24:40Z

lib/Sema/TypeCheckDecl.cpp

-        !isa<ModuleDecl>(accessScope.getDeclContext()) &&
-        TC.getLangOpts().isSwiftVersion3()) {
-      downgradeToWarning = DowngradeToWarning::Yes;
-    }


What happened to this code?

This was a separate patch -- it was duplicated twice so I moved it.

…and now it's applied to more places. Maybe it doesn't matter, though.

jrose-apple · 2018-05-18T16:25:10Z

lib/Sema/TypeCheckDecl.cpp

  }

  auto minAccess = minAccessScope.accessLevelForDiagnostics();

  bool isExplicit =
    owner->getAttrs().hasAttribute<AccessControlAttr>() ||
-    owner->getDeclContext()->getAsProtocolOrProtocolExtensionContext();
+    isa<ProtocolDecl>(owner->getDeclContext());


Oops, good catch! Is there a test for this?

Yep, see https://github.com/apple/swift/pull/16586/files#diff-8ace5fe1c031aa385557a1c025b63437

jrose-apple · 2018-05-18T16:28:03Z

lib/Sema/TypeCheckDecl.cpp

@@ -2175,14 +2223,16 @@ static void checkAccessControl(TypeChecker &TC, const Decl *D) {

  case DeclKind::Struct: {
    auto SD = cast<StructDecl>(D);
-    checkGenericParamAccess(TC, SD->getGenericParams(), SD);
+    checkGenericParamAccess(TC, SD->getGenericParams(), SD,
+                            /*checkUsableFromInline=*/false);


(here and elsewhere) Why is this false and not inherited? Can you leave a comment to that effect?

That's correct. checkAccessControl() only checks access control rules. The separate checkUsableFromInline() performs the same visitation of all TypeLocs, calling checkTypeAccess() with checkUsableFromInline=true.

jrose-apple · 2018-05-18T16:30:30Z

lib/Sema/TypeCheckDecl.cpp

+      if (anyVar->getFormalAccess() != AccessLevel::Internal)
+        return;
+      if (!anyVar->isUsableFromInline())
+        return;


This "internal and usable from inline" check shows up a lot. Maybe worth a helper function, even though it's so simple?

Yeah. I'm going to drop the assertion from isUsableFromInline() that the access level is internal, and just return false if its not.

jrose-apple · 2018-05-18T16:31:19Z

lib/Sema/TypeCheckDecl.cpp

+    };
+
+    std::for_each(assocType->getInherited().begin(),
+                  assocType->getInherited().end(),


This needs to check requirements, not just the inheritance clause, right? (Also, llvm::for_each.)

That's a good point. As you correctly noted this is copy and pasted from checkAccessControl() so probably the existing code was wrong too.

jrose-apple · 2018-05-18T16:32:25Z

lib/Sema/TypeCheckDecl.cpp

+      Type rawType = ED->getRawType();
+      auto rawTypeLocIter = std::find_if(ED->getInherited().begin(),
+                                         ED->getInherited().end(),
+                                         [&](TypeLoc inherited) {


Okay this is really starting to seem like too much code duplicated from the regular access checking.

Yeah. Perhaps we can factor out a visitor that walks the TypeLocs in a Decl?

jrose-apple · 2018-05-18T16:32:54Z

lib/Sema/TypeCheckDecl.cpp

@@ -4508,6 +4934,7 @@ class DeclChecker : public DeclVisitor<DeclChecker> {

    TC.checkDeclAttributes(SD);
    checkAccessControl(TC, SD);
+    checkUsableFromInline(TC, SD);


Do we need to do both? Can't checkAccessControl handle it?

Yes, I could merge the two into one function. It would eliminate the duplicated logic for visting the TypeLocs, at the cost of making the whole function longer. However, we could then split it up into smaller functions for each Decl kind or something, maybe even turn it into a visitor. Sounds good?

slavapestov · 2018-05-18T20:32:17Z

Source compat failure:

/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite/project_cache/Chatto/ChattoAdditions/Source/Chat Items/PhotoMessages/Views/PhotoMessageCollectionViewCellDefaultStyle.swift:92:36: error: type alias 'Class' is internal and cannot be referenced from a default argument value
        bubbleMasks: BubbleMasks = Class.createDefaultBubbleMasks(),
                                   ^
/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite/project_cache/Chatto/ChattoAdditions/Source/Chat Items/PhotoMessages/Views/PhotoMessageCollectionViewCellDefaultStyle.swift:28:15: note: type alias 'Class' is not public
    typealias Class = PhotoMessageCollectionViewCellDefaultStyle
              ^

Looks like I need to disable the new checks in -swift-version < 4.2.

jrose-apple · 2018-05-21T16:37:08Z

Less than 5, please. We don't currently have any language changes that are gated on 4.2 besides SDK changes, right?

jrose-apple

Comments for everything except the last Sema change.

jrose-apple · 2018-06-04T22:10:21Z

lib/Sema/TypeCheckDecl.cpp

@@ -2419,7 +2410,7 @@ static void checkAccessControl(TypeChecker &TC, const Decl *D) {
    if (!EED->hasAssociatedValues())
      return;
    for (auto &P : *EED->getParameterList()) {
-      checkTypeAccess(TC, P->getTypeLoc(), P,
+      checkTypeAccess(TC, P->getTypeLoc(), EED->getParentEnum(),


This one seems wrong; it should just be EED, right? (Because some day we might have enum cases less visible than the enum.)

Right. Fixed

jrose-apple · 2018-06-04T22:14:25Z

include/swift/AST/DiagnosticsSema.def

@@ -1317,20 +1328,25 @@ ERROR(unsupported_nested_protocol,none,
 // Type aliases
 ERROR(type_alias_underlying_type_access,none,
      "type alias %select{must be declared %select{"
-      "%select{private|fileprivate|internal|%error|%error}2|private or fileprivate}3"
+      "%select{private|fileprivate|internal|%error|%error}1|private or fileprivate}3"


I see: you changed the call site too. Please don't do this; it muddles the notion of what each parameter is used for. At that point I'd rather just use different diagnostics.

jrose-apple · 2018-06-04T22:16:57Z

lib/Sema/TypeCheckDecl.cpp

-        !isa<ModuleDecl>(accessScope.getDeclContext()) &&
-        TC.getLangOpts().isSwiftVersion3()) {
-      downgradeToWarning = DowngradeToWarning::Yes;
-    }


…and now it's applied to more places. Maybe it doesn't matter, though.

jrose-apple · 2018-06-04T22:17:36Z

lib/AST/Decl.cpp

@@ -2082,7 +2082,8 @@ SourceLoc ValueDecl::getAttributeInsertionLoc(bool forModifier) const {
 /// Returns true if \p VD needs to be treated as publicly-accessible
 /// at the SIL, LLVM, and machine levels due to being @usableFromInline.
 bool ValueDecl::isUsableFromInline() const {
-  assert(getFormalAccess() == AccessLevel::Internal);
+  if (getFormalAccess() != AccessLevel::Internal)
+    return false;


Unhappy with this. If you're going to make this an API that can be called everywhere, shouldn't it return true for open and public decls as well?

Slava and I talked about my remaining concerns in person.

Make this method public and fix it to do the right thing for associated types.

Previously we made sure that inlinable function bodies only reference @usableFromInline or public declarations, but we also have to make sure that @usableFromInline declarations only reference other declarations that are at least as visible. Otherwise, you could, for example, define a @usableFromInline typealias which referenced an internal type, and then reference the typealias from a @usableFromInline function body.

AccessControlCheckerBase |- AccessControlChecker |- UsableFromInlineChecker This factors out the checkUsableFromInline parameter that is passed around all over the place.

slavapestov · 2018-06-26T08:35:41Z

@swift-ci Please test

slavapestov · 2018-06-26T08:35:47Z

@swift-ci Please test source compatibility

slavapestov · 2018-06-26T08:39:11Z

This has been open for far too long. Since the latest iteration moves a lot of code to a new source file, I'm going to merge it now to avoid more merge conflicts. I'm not quite done extracting out the common visitor, but if you squint you will be able to guess where it's going to go.

slavapestov force-pushed the usable-from-inline-type-alias branch 4 times, most recently from ac2d25b to a0c6ca0 Compare May 18, 2018 08:13

slavapestov changed the title ~~[WIP] Check that signatures of @usableFromInline declarations only reference @usableFromInline or public types~~ Check that signatures of @usableFromInline declarations only reference @usableFromInline or public types May 18, 2018

slavapestov force-pushed the usable-from-inline-type-alias branch from a0c6ca0 to 6f9dc2c Compare May 18, 2018 09:04

slavapestov requested a review from jrose-apple May 18, 2018 09:23

jrose-apple previously requested changes May 18, 2018

View reviewed changes

slavapestov force-pushed the usable-from-inline-type-alias branch 5 times, most recently from 14ed713 to a68baa4 Compare June 3, 2018 08:37

jrose-apple reviewed Jun 4, 2018

View reviewed changes

This was referenced Jun 10, 2018

Minor @inlinable improvements #17090

Merged

Minor @inlinable improvements [4.2] #17091

Merged

Access control improvements #17252

Merged

slavapestov force-pushed the usable-from-inline-type-alias branch from a68baa4 to eb43b62 Compare June 15, 2018 21:10

slavapestov mentioned this pull request Jun 15, 2018

Sema: Type aliases referenced from @inlinable functions must be public or @usableFromInline #17257

Merged

slavapestov force-pushed the usable-from-inline-type-alias branch from eb43b62 to 155c94f Compare June 19, 2018 04:04

slavapestov mentioned this pull request Jun 25, 2018

stdlib: Update for stricter enforcement of @usableFromInline #17489

Merged

slavapestov force-pushed the usable-from-inline-type-alias branch from 155c94f to c241e38 Compare June 26, 2018 00:00

AST: Some tweaks for ValueDecl::isUsableFromInline()

aade63e

Make this method public and fix it to do the right thing for associated types.

slavapestov force-pushed the usable-from-inline-type-alias branch from c241e38 to 384aad4 Compare June 26, 2018 06:10

slavapestov added 4 commits June 26, 2018 00:31

Sema: Introduce AccessControlChecker

3707ebb

Sema: Move access control checking to TypeCheckAccess.cpp

7ae34aa

Sema: Rework AccessControlChecker into a class hierarchy

9b43f26

AccessControlCheckerBase |- AccessControlChecker |- UsableFromInlineChecker This factors out the checkUsableFromInline parameter that is passed around all over the place.

slavapestov force-pushed the usable-from-inline-type-alias branch from 384aad4 to 9b43f26 Compare June 26, 2018 08:34

slavapestov merged commit 0f25a3a into swiftlang:master Jun 26, 2018

slavapestov mentioned this pull request Jun 26, 2018

[SR-7471] Need to diagnose @usableFromInline stored property with inferred non-@usableFromInline type #50014

Closed

Check that signatures of @usableFromInline declarations only reference @usableFromInline or public types #16586

Check that signatures of @usableFromInline declarations only reference @usableFromInline or public types #16586

Uh oh!

Conversation

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

Uh oh!

Uh oh!

slavapestov commented May 18, 2018

Uh oh!

swift-ci commented May 18, 2018

Uh oh!

swift-ci commented May 18, 2018

Uh oh!

slavapestov commented May 18, 2018

Uh oh!

slavapestov commented May 18, 2018

Uh oh!

slavapestov commented May 18, 2018

Uh oh!

slavapestov commented May 18, 2018

Uh oh!

swift-ci commented May 18, 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!

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!

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!

slavapestov commented May 18, 2018

Uh oh!

jrose-apple commented May 21, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

slavapestov commented May 14, 2018 •

edited

Loading