Skip to content
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

Implement #warning and #error #14048

Merged
merged 8 commits into from
Feb 3, 2018
Merged

Conversation

harlanhaskins
Copy link
Collaborator

@harlanhaskins harlanhaskins commented Jan 22, 2018

This PR is an implementation of #warning and #error, which is currently being pitched to swift-evolution was accepted for Swift 5.

@harlanhaskins harlanhaskins added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Jan 22, 2018
@harlanhaskins
Copy link
Collaborator Author

@swift-ci please smoke test

@harlanhaskins
Copy link
Collaborator Author

This currently doesn't handle directives in the middle of switch statements, a la:

switch 4 {
#warning "boo"
default: break
}

@nkcsgexi
Copy link
Member

Finally... @harlanhaskins

@harlanhaskins harlanhaskins added swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process and removed swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review labels Feb 2, 2018
@harlanhaskins harlanhaskins changed the title [Do Not Merge] Implement #warning and #error [WIP] Implement #warning and #error Feb 2, 2018
@harlanhaskins
Copy link
Collaborator Author

@swift-ci please smoke test

@harlanhaskins
Copy link
Collaborator Author

@swift-ci please smoke test

Printer << tok::pound_warning;
}

Printer << '"' << PDD->getMessage()->getValue() << '"';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before I forget, I need to add parens to this.

@harlanhaskins
Copy link
Collaborator Author

@swift-ci please smoke test

@@ -188,6 +188,11 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*,
return false;
}

bool visitPoundDiagnosticDecl(PoundDiagnosticDecl *PDD) {
// By default, ignore #erorr/#warning.
Copy link

Choose a reason for hiding this comment

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

Typo on "erorr" :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!


auto string = parseExprStringLiteral();
if (string.isNull())
return makeParserError();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: continue looking for a close-paren if it occurs before the end of the line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

I originally tried checking for #warning “Foo” and FixIt-ing in the surrounding parens. Should I continue with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's also good, but I'm mostly thinking about #warning(Foo) and worse #warning(Foo is bad, yanno?), where the fix is probably to add quotes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also a good idea!

@@ -1098,6 +1098,17 @@ namespace {
PrintWithColorRAII(OS, ParenthesisColor) << ')';
}

void visitPoundDiagnosticDecl(PoundDiagnosticDecl *PDD) {
const char *name = PDD->isError() ?
"pound_error_decl" : "pound_warning_decl";
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: seems like having a common pound_diagnostic_decl as the kind and then printing an attribute for error vs. warning would be more inline with the other AST nodes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@@ -7143,6 +7144,14 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
TC.checkDeclAttributes(ICD);
}

void visitPoundDiagnosticDecl(PoundDiagnosticDecl *PDD) {
if (PDD->hasBeenEmitted()) { return; }
PDD->markEmitted();
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this can never happen more than once per phase, so you may be able to get rid of hasBeenEmitted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I’ll try it and see what happens!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turns out it does happen twice. ☹️

switch 34 {
#warning("warnings can be nested in switch statements") // expected-warning {{warnings can be nested in switch statements}}
#if true
#error("errors can be nested in if-configs inside switch statements too") // expected-error {{errors can be nested in if-configs inside switch statements too}}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a slightly different test where the #error is immediately after a case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, sorry, I guess that's just part of the body of the case. It's where the #if is immediately after a case, but contains cases itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I’ll nest a case inside the existing if config and then put a #error in there 👍

@harlanhaskins
Copy link
Collaborator Author

@jrose-apple Okay, this latest commit will fix-it:

#warning "foo"
#warning test 123
#warning(test 123)

@harlanhaskins
Copy link
Collaborator Author

@swift-ci please smoke test

@harlanhaskins
Copy link
Collaborator Author

@swift-ci please smoke test

// Catch #warning(oops, forgot the quotes)
SourceLoc wordsStartLoc = Tok.getLoc();

while (!Tok.isAtStartOfLine() && Tok.isNot(tok::r_paren)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to consume the r_paren!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ack! Thanks!

@harlanhaskins harlanhaskins changed the title [WIP] Implement #warning and #error Implement #warning and #error Feb 3, 2018
@rintaro
Copy link
Member

rintaro commented Feb 3, 2018

Could you add check for trailing tokens on the same line? e.g.

#warning("foo bar") var x = 1 // expected-error {{extra tokens following warning directive}}
#error("foo bar"); // expected-error {{extra tokens following error directive}}

I'm not sure this was discussed, but we don't allow this for any directives so far.

@harlanhaskins
Copy link
Collaborator Author

@swift-ci please smoke test

@harlanhaskins
Copy link
Collaborator Author

Build timed out?

@swift-ci please smoke test Linux platform

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This looks fantastic

@harlanhaskins
Copy link
Collaborator Author

🚢

@harlanhaskins harlanhaskins merged commit 5e02d2a into apple:master Feb 3, 2018
@@ -157,7 +157,7 @@ syn match swiftOperator "\.\.[<.]" skipwhite nextgroup=swiftTypeParameters
syn match swiftChar /'\([^'\\]\|\\\(["'tnr0\\]\|x[0-9a-fA-F]\{2}\|u[0-9a-fA-F]\{4}\|U[0-9a-fA-F]\{8}\)\)'/

syn match swiftPreproc /#\(\<file\>\|\<line\>\|\<function\>\)/
syn match swiftPreproc /^\s*#\(\<if\>\|\<else\>\|\<elseif\>\|\<endif\>\)/
syn match swiftPreproc /^\s*#\(\<if\>\|\<else\>\|\<elseif\>\|\<endif\>\<error\>\|\<warning\>\|\)/
Copy link
Member

Choose a reason for hiding this comment

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

This is what I call fit and finish 💯

@harlanhaskins harlanhaskins deleted the caution-tape branch February 3, 2018 23:27
@DevAndArtist
Copy link
Contributor

Is this supported?

#warning("""
  This is a very long warning \
  in a codebase that has 80 \
  character line width. ...
  """)

@harlanhaskins
Copy link
Collaborator Author

Not in the current implementation, but it absolutely should be. I’ll make a PR with that soon. Thanks for noticing!

@harlanhaskins
Copy link
Collaborator Author

Actually, spoke too soon. Multi line string literals are definitely supported already -- I mistakenly thought they were a different token.

@DougGregor
Copy link
Member

DougGregor commented Mar 9, 2018

Hey @harlanhaskins , mind adding a ChangeLog entry for this?

@harlanhaskins
Copy link
Collaborator Author

@DougGregor Submitted as #15110. Thanks for the reminder!

sl4m pushed a commit to sl4m/swift.vim that referenced this pull request Mar 2, 2020
* Implement #warning and #error

* Fix #warning/#error in switch statements

* Fix AST printing for #warning/#error

* Add to test case

* Add extra handling to ParseDeclPoundDiagnostic

* fix dumping

* Consume the right paren even in the failure case

* Diagnose extra tokens on the same line after a diagnostic directive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants