[AST] #SR-13906 (Improvement) Error Messages #35238

aniket-ray · 2020-12-29T17:29:12Z

Resolves SR-13906
https://bugs.swift.org/browse/SR-13906

Updated Error message for types trying to conform to AnyObject or its Composition for better user understandability.

For instance:

protocol P {}
typealias Q = P & AnyObject
struct S: Q {}
enum T: P & AnyObject {}
struct U: P, AnyObject {}

Before :

error: inheritance from non-protocol type 'Q' (aka 'P & AnyObject')
struct S: Q {}
       ^
error: inheritance from non-protocol type 'P & AnyObject'
enum T: P & AnyObject {}
     ^
error: inheritance from non-protocol, non-class type 'AnyObject'
class U: P, AnyObject {}
       ^

After :

error: Non-protocol 'S' cannot conform to composition 'Q' (aka 'P & AnyObject'). Composition 'Q' (aka 'P & AnyObject') involves 'AnyObject'
struct S: Q {}
       ^
error: Non-protocol 'T' cannot conform to composition 'P & AnyObject'. Composition 'P & AnyObject' involves 'AnyObject'
enum T: P & AnyObject {}
     ^
error: inheritance from non-protocol type 'AnyObject'
struct U: P, AnyObject {}
       ^

aniket-ray · 2020-12-29T17:30:38Z

Code Review and Suggestions: @CodaFi @augusto2112

augusto2112

Congratulations on your first PR! 🎉

augusto2112 · 2020-12-29T17:47:58Z

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -235,10 +235,16 @@ static void checkInheritanceClause(
      // AnyObject is not allowed except on protocols.
      if (layout.hasExplicitAnyObject &&
          !isa<ProtocolDecl>(decl)) {
-        decl->diagnose(canHaveSuperclass
+            if(!layout.isAnyObject()){


I think you should add a comment here explaining why this must be a composition (since we know that layout.hasExplicitAnyObject is true).

Added Comments

augusto2112 · 2020-12-29T17:49:03Z

lib/Sema/TypeCheckDeclPrimary.cpp

+            if(!layout.isAnyObject()){
+              decl->diagnose(diag::composition_of_protocol_and_AnyObject, typeDecl->getName() ,inheritedTy);            
+            }
+            else{


Did you run clang-format? I think the usual llvm style is:

if (condition) { } else { }

Ran clang-format and conformed to the LLVM Style.

CodaFi

This is a good start. I think we can make a few more tweaks and it'll be good to go.

CodaFi · 2020-12-29T22:32:22Z

include/swift/AST/DiagnosticsSema.def

@@ -2662,6 +2662,8 @@ ERROR(inheritance_from_cf_class,none,
 ERROR(inheritance_from_objc_runtime_visible_class,none,
      "cannot inherit from class %0 because it is only visible via the "
      "Objective-C runtime", (Identifier))
+ERROR(composition_of_protocol_and_AnyObject,none,
+      "Non-protocol %0 cannot conform to composition %1. Composition %1 involves 'AnyObject'", (Identifier, Type))


While I appreciate your good punctuation and grammar here, diagnostics have a distinct formatting style. They

don't begin with capital letters

rarely involve multiple sentences - when they run on, we use semicolons or break the second part into a note.

We have this document to lay out the common conventions.

We can do something like non-protocol type %0 cannot conform to composition type %1 and omit the Composition %1 involves 'AnyObject' part.

What do you think?

I still think we ought to highlight the AnyObject part of the composition in the message in some way. What might help is to think about this in isolation and come up with a "headline". That's usually a good way to write a diagnostic.

CodaFi · 2020-12-29T22:34:21Z

lib/Sema/TypeCheckDeclPrimary.cpp

+        // Check if the inherited type is exclusively AnyObject
+        // If not, we can be sure of the composition of Protocol(s) with
+        // AnyObject
+        if (!layout.isAnyObject()) {


I think it's more important to break up the diagnostic in the else by making it more specific and using less compiler-jargon-ified language.

You mean breaking down the ternary operator to if-else for canHaveSuperClass?

The intent of the original bug was to fix this particular ternary by providing a more specific message. Its branches are the cause of the confusion here.

LucianoPAlmeida · 2020-12-30T00:18:45Z

include/swift/AST/DiagnosticsSema.def

@@ -2662,6 +2662,8 @@ ERROR(inheritance_from_cf_class,none,
 ERROR(inheritance_from_objc_runtime_visible_class,none,
      "cannot inherit from class %0 because it is only visible via the "
      "Objective-C runtime", (Identifier))
+ERROR(composition_of_protocol_and_AnyObject,none,


Also, normally diagnostic ids are all lower case so this would be composition_of_protocol_and_anyobject :)

Updated Error message for types conforming to AnyObject or its Composition

aniket-ray · 2021-01-18T15:13:23Z

Testing Time: 5781.66s
  Unsupported      :  224
  Passed           : 6249
  Expectedly Failed:   24

5 warning(s) in tests

LucianoPAlmeida · 2021-01-18T22:43:47Z

test/Generics/inheritance.swift

@@ -55,7 +55,7 @@ func testGenericInherit() {
 }


-struct SS<T> : T { } // expected-error{{inheritance from non-protocol type 'T'}}
+struct SS<T> : T { } // expected-error{{generic struct type cannot inherit from non-protocol type 'T'giy }}


giy seems like a typo here :)

LucianoPAlmeida · 2021-01-18T22:55:04Z

test/decl/inherit/inherit.swift


 // Protocol inheriting a class
 protocol Q : A { }

 // Extension inheriting a class
-extension C : A { } // expected-error{{inheritance from non-protocol type 'A'}}
+extension C : A { } // expected-error{{extension type cannot inherit from non-protocol type 'A'}}


I think that there is no such term as extension type, to me this should be the same diagnostic as class C: A {} class type cannot inherit ...

LucianoPAlmeida · 2021-01-18T22:57:12Z

include/swift/AST/DiagnosticsSema.def

@@ -2662,6 +2662,11 @@ ERROR(inheritance_from_cf_class,none,
 ERROR(inheritance_from_objc_runtime_visible_class,none,
      "cannot inherit from class %0 because it is only visible via the "
      "Objective-C runtime", (Identifier))
+ERROR(composition_of_protocol_and_anyobject,none,


I see there is this tailored diagnostic for composition but not a regression test case for exercise it. We should add the respective test :)

augusto2112 · 2021-01-19T19:36:11Z

lib/Sema/TypeCheckDeclPrimary.cpp

+          decl->diagnose(diag::composition_of_protocol_and_anyobject,
+                         typeDecl->getName(), inheritedTy);
+        } else {
+          decl->diagnose(canHaveSuperclass


I think the comment left by Codafi, of addressing the else block still needs to be done.

xwu · 2023-04-27T04:07:38Z

Looks like #41537 has addressed SR-13906, superseding this PR.

augusto2112 requested changes Dec 29, 2020

View reviewed changes

aniket-ray force-pushed the Improvement_SR-13906_ErrorMessage branch from ef3916a to 85b66bf Compare December 29, 2020 21:33

CodaFi requested changes Dec 29, 2020

View reviewed changes

LucianoPAlmeida reviewed Dec 30, 2020

View reviewed changes

Aniket Ray added 2 commits January 18, 2021 09:27

[AST] #SR-13906 (Improvement)

b2007cf

Updated Error message for types conforming to AnyObject or its Composition

Updated Tests for Diagnostics

aad3bc6

aniket-ray force-pushed the Improvement_SR-13906_ErrorMessage branch from 85b66bf to aad3bc6 Compare January 18, 2021 15:10

aniket-ray requested review from CodaFi and LucianoPAlmeida January 18, 2021 15:13

LucianoPAlmeida reviewed Jan 18, 2021

View reviewed changes

augusto2112 reviewed Jan 19, 2021

View reviewed changes

xwu closed this Apr 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AST] #SR-13906 (Improvement) Error Messages #35238

[AST] #SR-13906 (Improvement) Error Messages #35238

aniket-ray commented Dec 29, 2020 •

edited

Loading

aniket-ray commented Dec 29, 2020

augusto2112 left a comment

augusto2112 Dec 29, 2020

aniket-ray Dec 29, 2020

augusto2112 Dec 29, 2020 •

edited

Loading

aniket-ray Dec 29, 2020

CodaFi left a comment

CodaFi Dec 29, 2020

CodaFi Dec 29, 2020

aniket-ray Dec 30, 2020

CodaFi Jan 6, 2021

CodaFi Dec 29, 2020

aniket-ray Dec 30, 2020

CodaFi Jan 6, 2021

LucianoPAlmeida Dec 30, 2020

aniket-ray Dec 30, 2020

aniket-ray commented Jan 18, 2021

LucianoPAlmeida Jan 18, 2021

LucianoPAlmeida Jan 18, 2021

LucianoPAlmeida Jan 18, 2021

augusto2112 Jan 19, 2021 •

edited

Loading

xwu commented Apr 27, 2023

[AST] #SR-13906 (Improvement) Error Messages #35238

[AST] #SR-13906 (Improvement) Error Messages #35238

Conversation

aniket-ray commented Dec 29, 2020 • edited Loading

aniket-ray commented Dec 29, 2020

augusto2112 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augusto2112 Dec 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaFi 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

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

aniket-ray commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augusto2112 Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

xwu commented Apr 27, 2023

aniket-ray commented Dec 29, 2020 •

edited

Loading

augusto2112 Dec 29, 2020 •

edited

Loading

augusto2112 Jan 19, 2021 •

edited

Loading