ASTPrinter: Mutability fixes for protocol stubs #24277

AnthonyLatsis · 2019-04-25T19:29:46Z

Don't print mutating in protocol stubs inside classes (this did happen to stubs for mutating requirements when the protocol and conformer were in different modules).
Print nonmutating for non-mutating setters in structs.

The AST printer would benefit from a better distinction between attributes and modifiers, but I think it is fine to implement it this way, for now :)

AnthonyLatsis · 2019-04-25T20:39:01Z

cc @jrose-apple

jrose-apple · 2019-04-25T23:12:11Z

This seems wrong: we do want to print mutating on stubs by default, because usually they don't have a sensible non-mutating implementation.

AnthonyLatsis · 2019-04-25T23:51:26Z

I see, that makes sense. I got too carried away reasoning from a different perspective: that mutating (or, say, throws) on stubs could give the impression of being a mandatory modifier, when such requirements are ~~often~~(«sometimes» is probably more accurate) expected to be understood as optional (spare flexibility). On the other hand, mutating methods with proper names should speak for themselves.

I will enable it on structs in a moment.

jrose-apple · 2019-04-26T00:41:45Z

Enums too; it's classes that are special.

I think the stubs are supposed to be "a reasonable default", but I could see how they could be interpreted as "the minimum set of requirements".

AnthonyLatsis · 2019-05-01T15:45:24Z

@jrose, do you approve?

jrose-apple

Thanks for working on this!

jrose-apple · 2019-05-01T15:49:38Z

lib/Sema/TypeCheckProtocol.cpp

+    if (isClass)
+      Options.ExcludeAttrList.push_back(DAK_Mutating);
+    // Struct methods are non-mutating be default.
+    if (isClass || !isa<AbstractStorageDecl>(Requirement))


I don't think this qualification is necessary. You're not allowed to write nonmutating on a non-accessor anyway. May as well just put both conditions under isClass.

You're not allowed to write nonmutating on a non-accessor anyway

Am I? nonmutating is allowed on protocol method requirements, which makes sense, but it appears we can also write it on value type methods in Swift 5, which is obviously redundant.

Um. It doesn't even make sense for protocol requirements, but you're right that it's not rejected. I did not realize that.

It doesn't even make sense for protocol requirements

Oh, right, of course.

jrose-apple · 2019-05-01T15:50:07Z

lib/Sema/TypeCheckProtocol.cpp

@@ -2657,6 +2657,16 @@ printRequirementStub(ValueDecl *Requirement, DeclContext *Adopter,
    Options.FunctionDefinitions = true;
    Options.PrintAccessorBodiesInProtocols = true;

+    bool isClass = AdopterTy->is<ClassType>();


This will only be true for non-generic classes. A simpler way to check is AdopterTy->getSelfClassDecl() != nullptr.

Oops, ClassType is indeed misleading, thanks!

jrose-apple · 2019-05-01T15:50:40Z

test/decl/protocol/conforms/Inputs/fixit_stub_mutability_proto_module.swift

@@ -0,0 +1,4 @@
+public protocol ExternalMutabilityProto {
+    mutating func foo()
+    subscript() -> Int { get nonmutating set }


If you want to be extra thorough, you could check how mutating get prints as well.

I also noticed that we never print accessors for property requirements, even when the getter is required to be mutating or the setter - non-mutating. Imagine how confusing a conformance error can be in the non-mutating case.

The getter can't be required to be mutating, just allowed to be mutating. But the setter case would force you into a computed property, and so the stubs for that could include the computed property structure. Let's save that for another PR, though.

jrose-apple · 2019-05-01T15:51:57Z

lib/AST/ASTPrinter.cpp

@@ -2768,7 +2768,7 @@ void PrintAST::visitFuncDecl(FuncDecl *decl) {
    if (!Options.SkipIntroducerKeywords) {
      if (decl->isStatic() && Options.PrintStaticKeyword)
        printStaticKeyword(decl->getCorrectStaticSpelling());
-      if (decl->isMutating() && !decl->getAttrs().hasAttribute<MutatingAttr>()) {


Is this part not relevant? I assumed it kept mutating from being printed twice in some cases.

Not anymore I believe, since I chose to skip mutability modifiers in printAttributes.

FYI this caused interface generation to print mutating mutating for explicitly mutating functions, because the changes made to printAttributes only applied to implicit attributes. Fixed: #30183

jrose-apple · 2019-05-06T17:23:45Z

@swift-ci Please test

swift-ci · 2019-05-06T17:24:05Z

Build failed
Swift Test Linux Platform
Git Sha - 0173e1edb017d6038e5ec48129237cf275b3b434

swift-ci · 2019-05-06T17:28:01Z

Build failed
Swift Test OS X Platform
Git Sha - 0173e1edb017d6038e5ec48129237cf275b3b434

jrose-apple · 2019-05-06T20:25:19Z

:-( Looks like the duplicate mutating was a problem after all.

AnthonyLatsis · 2019-10-25T15:32:03Z

Not anymore I believe, since I chose to skip mutability modifiers in printAttributes.

@jrose Turns out I forgot to skip them for functions as well. I don't know which tests failed now that the job is gone, though. Let's see if the duplication is still present?

theblixguy · 2019-10-25T16:18:01Z

@swift-ci please test

jrose-apple · 2019-10-25T17:19:44Z

Looks pretty good now. Are there already tests for mutating get in a requirement, which…could probably go either way.

AnthonyLatsis · 2019-10-25T17:47:31Z

Yes, though I do print mutating for both getters and methods, because they are both "allowed to be mutating" in that case.

swift-ci · 2019-10-25T17:53:28Z

Build failed
Swift Test OS X Platform
Git Sha - 6f119b0

jrose-apple · 2019-10-25T18:12:09Z

That LLDB test was recently disabled, let's go again.

@swift-ci Please smoke test macOS

AnthonyLatsis · 2020-03-03T19:29:28Z

Fix double-printing mutating on interface generation: #30011
Sorry about that :-/

AnthonyLatsis force-pushed the mutability-proto-stubs branch from cfc0148 to b7d4f61 Compare April 25, 2019 19:32

AnthonyLatsis force-pushed the mutability-proto-stubs branch 2 times, most recently from d77dbf7 to 0173e1e Compare April 26, 2019 13:43

jrose-apple reviewed May 1, 2019

View reviewed changes

AnthonyLatsis added 2 commits October 25, 2019 18:19

ASTPrinter: Mutability fixes for protocol stubs

1892088

Test protocol stubs for mutating getters

6f119b0

AnthonyLatsis force-pushed the mutability-proto-stubs branch from 589d66a to 6f119b0 Compare October 25, 2019 15:31

jrose-apple merged commit dfe00c3 into swiftlang:master Oct 25, 2019

AnthonyLatsis mentioned this pull request Oct 26, 2019

Sema: Print property accessors for non-mutating setter requirements #27896

Merged

AnthonyLatsis deleted the mutability-proto-stubs branch February 17, 2022 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASTPrinter: Mutability fixes for protocol stubs #24277

ASTPrinter: Mutability fixes for protocol stubs #24277

AnthonyLatsis commented Apr 25, 2019 •

edited

Loading

AnthonyLatsis commented Apr 25, 2019

jrose-apple commented Apr 25, 2019

AnthonyLatsis commented Apr 25, 2019 •

edited

Loading

jrose-apple commented Apr 26, 2019

AnthonyLatsis commented May 1, 2019

jrose-apple left a comment

jrose-apple May 1, 2019

AnthonyLatsis May 2, 2019

jrose-apple May 2, 2019

AnthonyLatsis May 4, 2019 •

edited

Loading

jrose-apple May 1, 2019

AnthonyLatsis May 2, 2019

jrose-apple May 1, 2019

AnthonyLatsis May 2, 2019

jrose-apple May 2, 2019

jrose-apple May 1, 2019

AnthonyLatsis May 4, 2019

benlangmuir Mar 3, 2020

jrose-apple commented May 6, 2019

swift-ci commented May 6, 2019

swift-ci commented May 6, 2019

jrose-apple commented May 6, 2019

AnthonyLatsis commented Oct 25, 2019

theblixguy commented Oct 25, 2019

jrose-apple commented Oct 25, 2019 •

edited

Loading

AnthonyLatsis commented Oct 25, 2019 •

edited

Loading

swift-ci commented Oct 25, 2019

jrose-apple commented Oct 25, 2019

AnthonyLatsis commented Mar 3, 2020 •

edited

Loading

ASTPrinter: Mutability fixes for protocol stubs #24277

ASTPrinter: Mutability fixes for protocol stubs #24277

Conversation

AnthonyLatsis commented Apr 25, 2019 • edited Loading

AnthonyLatsis commented Apr 25, 2019

jrose-apple commented Apr 25, 2019

AnthonyLatsis commented Apr 25, 2019 • edited Loading

jrose-apple commented Apr 26, 2019

AnthonyLatsis commented May 1, 2019

jrose-apple left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyLatsis May 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrose-apple commented May 6, 2019

swift-ci commented May 6, 2019

swift-ci commented May 6, 2019

jrose-apple commented May 6, 2019

AnthonyLatsis commented Oct 25, 2019

theblixguy commented Oct 25, 2019

jrose-apple commented Oct 25, 2019 • edited Loading

AnthonyLatsis commented Oct 25, 2019 • edited Loading

swift-ci commented Oct 25, 2019

jrose-apple commented Oct 25, 2019

AnthonyLatsis commented Mar 3, 2020 • edited Loading

AnthonyLatsis commented Apr 25, 2019 •

edited

Loading

AnthonyLatsis commented Apr 25, 2019 •

edited

Loading

AnthonyLatsis May 4, 2019 •

edited

Loading

jrose-apple commented Oct 25, 2019 •

edited

Loading

AnthonyLatsis commented Oct 25, 2019 •

edited

Loading

AnthonyLatsis commented Mar 3, 2020 •

edited

Loading