[SR-12460] Don't crash when missing a type in a Diagnostic message. #30979

blueeor · 2020-04-11T20:37:22Z

Print an empty string for the missing type.

Given this swift file:

extension {
  func ==(lhs: Foo, rhs: Foo) -> Bool {}
}

And the swift arguments that XCode uses to compile swift.

Produce this output:

/Users/eyork/dev/test/Test/Test/main.swift:13:11: error: expected type name in extension declaration
extension {
          ^
/Users/eyork/dev/test/Test/Test/main.swift:14:8: **error: operator '==' declared in extension of '' must be 'static'**
  func ==(lhs: Foo, rhs: Foo) -> Bool {}
       ^
  static 
/Users/eyork/dev/test/Test/Test/main.swift:14:16: error: use of undeclared type 'Foo'
  func ==(lhs: Foo, rhs: Foo) -> Bool {}
               ^~~
/Users/eyork/dev/test/Test/Test/main.swift:14:26: error: use of undeclared type 'Foo'
  func ==(lhs: Foo, rhs: Foo) -> Bool {}
                         ^~~
Program ended with exit code: 1

This line in bold has the fixed result. It will refer to the missing type as ''.
Before this fix, the compiler would crash when producing that line.

Print an empty string for the missing type. Resolves SR-12460

xedin

Thank you for the contribution! I'd suggest we actually fix declaration checker to verify that extension has a name before trying to produce this diagnostic about operator and add an assert in DiagnosticEngine to make sure that TypeRepr * is not null.

Diagnostic for non-static operator declaration is emitted here https://github.com/apple/swift/blob/master/lib/Sema/TypeCheckDecl.cpp#L679-L682. I think it might be worth adding a new diagnostic message which doesn't require a name and emit that instead if situations like this.

blueeor · 2020-04-12T02:08:29Z

Ok, I will make those updates.

…n must be 'static'" Added assert for missing a type name IsStaticRequest::evaluate now checks for ED->getExtendedTypeRepr().

blueeor · 2020-04-12T06:09:51Z

Made that update. The new diagnostic message is "operator '==' declared in extension must be 'static''. How does that look?

AnthonyLatsis

Good catch! Could you please add a test so that we don't regress again? swift/test/decl/ext/extensions.swift is a good spot.

include/swift/AST/DiagnosticsSema.def

lib/AST/DiagnosticEngine.cpp

lib/Sema/TypeCheckDecl.cpp

Formatting updates

blueeor · 2020-04-13T00:45:25Z

I added a test in swift/test/decl/ext/extensions.swift but I am not sure how to run that single test.

AnthonyLatsis

Looks good, thank you! Have a look at https://github.com/apple/swift/blob/master/docs/Testing.md#using-litpy for how to use lit to run the tests. --filter= — to run a single test file.

lib/Sema/TypeCheckDecl.cpp

AnthonyLatsis

I have one final suggestion, the rest is up to @xedin.

test/decl/ext/extensions.swift

xedin

LGTM, thank you!

xedin · 2020-04-13T16:52:05Z

@swift-ci please smoke test

xedin · 2020-04-13T17:45:53Z

@blueeor Looks like there are test failures.

blueeor · 2020-04-13T17:49:43Z

ok, I will look at that.

blueeor · 2020-04-13T20:29:25Z

The test fails for swift 4:
RUN: at line 1'; /Users/eyork/apple-swift/build/Ninja-DebugAssert/swift-macosx-x86_64/bin/swiftc -frontend -target x86_64-apple-macosx10.9 -module-cache-path '/Users/eyork/apple-swift/build/Ninja-DebugAssert/swift-macosx-x86_64/swift-test-results/x86_64-apple-macosx10.9/clang-module-cache' -sdk '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk' -swift-version 4 -ignore-module-source-info -typo-correction-limit 10 -typecheck -verify -disable-objc-attr-requires-foundation-module /Users/eyork/apple-swift/swift/test/decl/ext/extensions.swift

I am assuming that it's working correctly for swift 5.

I don't know how to set the error test in extensions.swift to only be for swift 5?

xedin · 2020-04-13T20:31:01Z

Why does it fail for Swift 4 and not 5?

blueeor · 2020-04-13T23:57:27Z

I was mistaken. This doesn't have anything to do with swift language 4 or 5.

This test is running swiftc with different options vs when xcode runs swift. With the default options on swift, this diagnosis message doesn't appear. I haven't yet figured out which swift option will make this diagnosis message appear (a pointer in the right direction would help me). The other question is would I need to put this test in it's own file and add a test that runs swiftc with the correct options? Or is there already a test that runs swift with the same options that XCode would use?

xedin · 2020-04-14T00:15:58Z

@blueeor I think it would be fine to add a new test file with particular set of flags in RUN which reproduce this problem, you can use SR number as a file name, we do that in test-suite already.

AnthonyLatsis · 2020-04-14T00:29:06Z

The code surrounding the emission of the error is not showing any dependence on an invocation flag, and the diagnostic does appear with a proper extension; it has to be something else.

LucianoPAlmeida · 2020-04-14T00:31:50Z

I haven't yet figured out which swift option will make this diagnosis message appear (a pointer in the right direction would help me).

@blueeor From the stack-trace seems like the crash appears when trying to index symbols (IndexSwiftAST), so you can pass something like target-swift-ide-test -print-indexed-symbols
could be that @xedin?

Edit: RUN: %target-swift-ide-test -print-indexed-symbols -source-filename %s seems to make it :)

AnthonyLatsis · 2020-04-14T01:21:07Z

OK, so in between recent snapshots and the Xcode 11.3 release something happened that caused us to completely skip validation of members inside these anonymous extensions, which is why the error is not emitted normally in this case. We either have to fix that or hit the code path by using the appropriate command to emit index data (@LucianoPAlmeida pointed out the right one).

blueeor · 2020-04-14T01:49:14Z

@LucianoPAlmeida Thanks. I will update the test with that or I guess make a new test for this.

@AnthonyLatsis It sounds like you are saying there is a larger bug at play here?

blueeor · 2020-04-14T02:16:53Z

I updated the tests and it passes for me now. I pushed the commit. Will the ci system pick this up or does it need a prompt to run again?

LucianoPAlmeida · 2020-04-14T02:23:00Z

I updated the tests and it passes for me now. I pushed the commit. Will the ci system pick this up or does it need a prompt to run again?

@blueeor Someone with the rights should kick the CI
cc @xedin :)

AnthonyLatsis · 2020-04-14T02:31:36Z

test/decl/ext/extensions_no_type.swift

+//===--- Test that we don't crash when validating members inside an extension with no type name.
+extension { // expected-error {{expected type name in extension declaration}}
+  static func ==(lhs: Foo, rhs: Foo) -> Bool {} // expected-error {{operator '==' declared in extension must be 'static'}}
+}


I think we should move the test to test/Index now that we use this command.
Also, since the test is somewhat alienated, can you name it after the SR to provide more context? This will make it easier to move back to decl/ext once we fix the validation issue.

Oh no, the crash is guarded, but the expected-error checks here will have no effect. The only way to test that your error is actually there is to CHECK: for it in the command output. That's a pretty dirty workaround, so let's wait for what Pavel has to say.

I renamed the the test file to SR-12460.swift.

I don't understand your comment on CHECK:, the test seems to work correctly with those compile options? What I am missing?

I don't understand your comment on CHECK:, the test seems to work correctly with those compile options? What I am missing?

The test will still pass if you mess up the expected errors. You need %target-typecheck-verify-swift for the expected-error check to take effect, but the error you want is only emitted as a byproduct of the %target-swift-ide-test -print-indexed-symbols output.

I don’t think typecheck-verify command is really necessary here, this output could be verified with FileCheck. I’ll make some experiments tomorrow and let you know.

xedin · 2020-04-14T21:07:04Z

Here is the test-case we can use for this issue, let's have it as test/decl/ext/sr_12460.swift

// RUN: %target-swift-ide-test -print-indexed-symbols -source-filename %s 2>&1 | %FileCheck -check-prefix CHECK %s

//Test that we don't crash when validating members inside an extension with no type name.

// CHECK: :[[@LINE+1]]:11: error: expected type name in extension declaration
extension {
  // CHECK: :[[@LINE+1]]:8: error: operator '==' declared in extension must be 'static'
  func ==(lhs: Any, rhs: Any) -> Bool {}
}

blueeor · 2020-04-14T21:20:05Z

@xedin I updated the test file with those changes.

xedin · 2020-04-14T21:28:23Z

@swift-ci please smoke test

xedin · 2020-04-14T23:13:28Z

Thank you, @blueeor! Please don't forget to resolve associated SR.

Don't crash when missing a type in a Diagnostic message.

bfaeab3

Print an empty string for the missing type. Resolves SR-12460

blueeor changed the title ~~Don't crash when missing a type in a Diagnostic message.~~ [SR-12460] Don't crash when missing a type in a Diagnostic message. Apr 11, 2020

xedin requested changes Apr 12, 2020

View reviewed changes

Added new diagnostic message, eg. "operator '==' declared in extensio…

ca01957

…n must be 'static'" Added assert for missing a type name IsStaticRequest::evaluate now checks for ED->getExtendedTypeRepr().

blueeor added 2 commits April 11, 2020 23:11

Format update

442068f

Moved if check

f2e927d

blueeor requested a review from xedin April 12, 2020 06:15

AnthonyLatsis reviewed Apr 12, 2020

View reviewed changes

include/swift/AST/DiagnosticsSema.def Outdated Show resolved Hide resolved

lib/AST/DiagnosticEngine.cpp Outdated Show resolved Hide resolved

lib/AST/DiagnosticEngine.cpp Outdated Show resolved Hide resolved

AnthonyLatsis reviewed Apr 12, 2020

View reviewed changes

lib/Sema/TypeCheckDecl.cpp Outdated Show resolved Hide resolved

blueeor added 3 commits April 12, 2020 10:03

Updated alert message to "TypeRepr argument is null"

01d0394

Formatting updates

Moved diagnostic to one message

a6c6c09

Add test for extention without type

3ed5315

blueeor requested a review from AnthonyLatsis April 13, 2020 00:46

AnthonyLatsis reviewed Apr 13, 2020

View reviewed changes

lib/Sema/TypeCheckDecl.cpp Outdated Show resolved Hide resolved

Use nullptr

830ec43

blueeor requested a review from AnthonyLatsis April 13, 2020 03:30

AnthonyLatsis reviewed Apr 13, 2020

View reviewed changes

test/decl/ext/extensions.swift Outdated Show resolved Hide resolved

Update test comment.

623e08f

blueeor requested a review from AnthonyLatsis April 13, 2020 04:27

xedin approved these changes Apr 13, 2020

View reviewed changes

Fixed test to add target-swift-ide-test -print-indexed-symbols

f3ce4d6

AnthonyLatsis reviewed Apr 14, 2020

View reviewed changes

blueeor added 2 commits April 13, 2020 20:29

Updated test file name

eb45bb0

Updated test file name

99af14a

blueeor requested a review from xedin April 14, 2020 03:34

Update test file

06a0650

xedin merged commit 8a1a194 into swiftlang:master Apr 14, 2020

xedin approved these changes Apr 14, 2020

View reviewed changes

swift-ci mentioned this pull request Apr 11, 2020

[SR-12460] Declaring an operator inside an extension without a name crashes the compiler #54900

Closed

[SR-12460] Don't crash when missing a type in a Diagnostic message. #30979

[SR-12460] Don't crash when missing a type in a Diagnostic message. #30979

Uh oh!

Conversation

blueeor commented Apr 11, 2020

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

blueeor commented Apr 12, 2020

Uh oh!

blueeor commented Apr 12, 2020

Uh oh!

AnthonyLatsis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blueeor commented Apr 13, 2020

Uh oh!

AnthonyLatsis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AnthonyLatsis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

xedin commented Apr 13, 2020

Uh oh!

xedin commented Apr 13, 2020

Uh oh!

blueeor commented Apr 13, 2020

Uh oh!

blueeor commented Apr 13, 2020

Uh oh!

xedin commented Apr 13, 2020

Uh oh!

blueeor commented Apr 13, 2020

Uh oh!

xedin commented Apr 14, 2020

Uh oh!

AnthonyLatsis commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LucianoPAlmeida commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnthonyLatsis commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueeor commented Apr 14, 2020

Uh oh!

blueeor commented Apr 14, 2020

Uh oh!

LucianoPAlmeida commented Apr 14, 2020

Uh oh!

AnthonyLatsis Apr 14, 2020 • 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!

AnthonyLatsis Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

blueeor Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

AnthonyLatsis Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

xedin Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

xedin commented Apr 14, 2020

Uh oh!

AnthonyLatsis commented Apr 14, 2020 •

edited

Loading

LucianoPAlmeida commented Apr 14, 2020 •

edited

Loading

AnthonyLatsis commented Apr 14, 2020 •

edited

Loading

AnthonyLatsis Apr 14, 2020 •

edited

Loading