Skip to content

[Diagnostic verifier] Reject diagnostics at '<unknown>:0' by default #7197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 7, 2017

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Feb 2, 2017

Previously, in -verify mode, diagnostics for invalid locations were silently ignored.
However, most of them are unintentional, and unhelpful for users.
I think we should eliminate them where possible.

In this PR, -verify mode catch them and report them as failure by default.
Added -verify-ignore-unknown frontend option for opting-out this feature.

@rintaro rintaro force-pushed the diagverify-unknown branch from d231a7b to 019daba Compare February 2, 2017 01:49
@rintaro
Copy link
Member Author

rintaro commented Feb 2, 2017

@jrose-apple What do you think?

// <unknown>:0: error: unexpected error produced: only classes and class members may be marked with 'final'
// <unknown>:0: error: unexpected error produced: only classes and class members may be marked with 'final'
// <unknown>:0: error: unexpected error produced: only classes and class members may be marked with 'final'
// <unknown>:0: error: unexpected error produced: only classes and class members may be marked with 'final'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#7179 will fix this.

@jrose-apple
Copy link
Contributor

Ooh, this is a good idea, although I'm sad about how many we seem to have. I'd like someone else to take a look too, maybe @bitjammer or @modocache?

@rintaro
Copy link
Member Author

rintaro commented Feb 2, 2017

I think most of them are missing implementation of importSourceLoc() in ClangImporter.
I don't know how to implement that though 😓

@jrose-apple
Copy link
Contributor

At this point I'm not sure we really consider that "missing"; we really shouldn't emit errors or warnings on imported declarations, ever.

@slavapestov
Copy link
Contributor

@jrose-apple What about a "note: declared here" type of thing? That should use the decl as the location so it prints correctly right?

@jrose-apple
Copy link
Contributor

That's the intent, yes—any of those notes should result in printing into a synthetic buffer for now. We've also long talked about having an "IDE mode" where a USR is used as the "location", so that Xcode can actually show the generated interface as it usually does. In neither case is the location reported as "unknown".

@rintaro
Copy link
Member Author

rintaro commented Feb 4, 2017

@swift-ci Please test

@DougGregor
Copy link
Member

This looks good to me. Starter bug: eliminate -verify-ignore-unknown from some tests :)

@rintaro
Copy link
Member Author

rintaro commented Feb 7, 2017

@swift-ci Please smoke test and merge

@swift-ci swift-ci merged commit 5d2a185 into swiftlang:master Feb 7, 2017
@rintaro rintaro deleted the diagverify-unknown branch February 9, 2017 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants