Skip to content

Add compiler warnings for redundant arguments in raise/failwith/failwithf/nullArg/invalidOp/invalidArg#630

Closed
dungpa wants to merge 5 commits intodotnet:masterfrom
dungpa:failwith-warning
Closed

Add compiler warnings for redundant arguments in raise/failwith/failwithf/nullArg/invalidOp/invalidArg#630
dungpa wants to merge 5 commits intodotnet:masterfrom
dungpa:failwith-warning

Conversation

@dungpa
Copy link
Copy Markdown
Contributor

@dungpa dungpa commented Sep 11, 2015

Close #46.

  • Added a post inference check for redundant arguments of these built-in functions: raise, failwith, failwithf, nullArg, invalidOp and invalidArg.
  • Added relevant tests to FSharpQA test suite.
  • Revised a few usages in the compiler, core libraries that raise warnings as errors due to adding the feature.

Points for discussion:

  • What is the good range to add the warning number? At the moment, I just append to FSComp.txt.
  • To be able to support varied number of arguments in failwithf, I have to run ParseFormatString one more time. Is there any place to get argument count of failwithf usage? At the point of this check, the full type checking phase is not yet executed.
  • The warning message could be improved.

Here is how the feature is in action.

image

@latkin
Copy link
Copy Markdown
Contributor

latkin commented Sep 16, 2015

LGTM. Nice work, very useful! Your solution to the open questions looks reasonable to me, though of course others are welcome to comment.

@latkin latkin closed this in 499b2c3 Sep 16, 2015
@latkin latkin added the fixed label Sep 16, 2015
@latkin latkin added this to the F# 4.0 Update 1 milestone Sep 16, 2015
@latkin
Copy link
Copy Markdown
Contributor

latkin commented Sep 16, 2015

This is a breaking change as it introduces new warnings to existing warning-free code, but it seems so obviously correct that IMO it's worth taking. We can roll back if others feel strongly otherwise.

I think the error message is reasonable.

@dungpa dungpa deleted the failwith-warning branch September 17, 2015 05:40
@dungpa
Copy link
Copy Markdown
Contributor Author

dungpa commented Sep 17, 2015

@latkin Thanks for merging. I guess I have to pick new issues to work on :-).

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.

3 participants