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

Change the warning statement for unused result from expression #12266

Closed
wants to merge 3 commits into from

Conversation

fluffybeing
Copy link
Contributor

Resolves SR-5983.

@huonw
Copy link
Contributor

huonw commented Oct 4, 2017

Thanks!

The compiler also has tests for diagnostics like this, using directives like expected-warning {{...}} and expected-error {{...}} to tell the testing harness that the compiler should emit a specific message attached to the line with the comment (or, with expected-warning @number on the line number before or after). There's a few tests that look for this one, could you update them too?

test/attr/attr_discardableResult.swift
152:  a() // expected-warning {{result of call is unused, but produces 'Int'}}

test/ClangImporter/objc_parse.swift
217:  // expected-warning @-1 {{result of call is unused, but produces 'Unmanaged<AnyObject>!'}}

test/Constraints/diagnostics.swift
340:f8(10)          // expected-warning {{result of call is unused, but produces 'Int'}}

test/expr/expressions.swift
674:  fn(&n, 12) // expected-warning {{result of call is unused, but produces 'Int'}}

test/Parse/recovery.swift
131:  } while { true }() // expected-error{{missing condition in a 'while' statement}} expected-error{{consecutive statements on a line must be separated by ';'}} {{10-10=;}} expected-warning {{result of call is unused, but produces 'Bool'}}

@@ -2814,7 +2814,7 @@ WARNING(expression_unused_result_call,none,
WARNING(expression_unused_result_operator,none,
"result of operator %0 is unused", (DeclName))
WARNING(expression_unused_result_unknown, none,
"result of call is unused, but produces %0", (Type))
"result of call to function returning %0 is unused", (Type))
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You may not necessarily be calling a function if this is emitted, you may be calling a closure.

I think you can probably delete this and just use expression_unused_result_call if it's all the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CodaFi I will update it. I think the explicit information will be good and so I will go with your first suggestion.

@fluffybeing
Copy link
Contributor Author

@huonw thanks for the help. Also, I was curious that how can I run the test locally. Wasn't able to figure out from the documentation.

@fluffybeing
Copy link
Contributor Author

@huonw am I missing something? or this PR is ready for acceptance.

@@ -2814,7 +2814,7 @@ WARNING(expression_unused_result_call,none,
WARNING(expression_unused_result_operator,none,
"result of operator %0 is unused", (DeclName))
WARNING(expression_unused_result_unknown, none,
"result of call is unused, but produces %0", (Type))
"result of call to closure returning %0 is unused", (Type))
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 not quite what I was pointing out - not everything this diagnoses is a call to a closure, nor is everything it diagnoses a call to a function. So you have two options:

  1. Conditionalize this diagnostic on whether the called value is a closure or a function.
  2. Just delete it and use expression_unused_result_call instead.

I really recommend #2. This diagnostic seems genuinely superfluous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CodaFi Thanks for the context. I thought it is just for closure and I will also not prefer conditionalizing it. I will update it then.

@CodaFi
Copy link
Member

CodaFi commented Nov 11, 2019

This was addressed by #16059. Thanks!

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.

None yet

3 participants