Add -fdefer-out-of-scope-variables flag (#12170).
ClosedPublic

Authored by ak3n on Aug 17 2016, 7:33 PM.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ak3n updated this revision to Diff 8424.Aug 17 2016, 7:33 PM
ak3n retitled this revision from to Add -fdefer-out-of-scope-variables flag (#12170)..
ak3n updated this object.
ak3n edited the test plan for this revision. (Show Details)
ak3n added a comment.Aug 17 2016, 7:54 PM

Is there need for Opt_WarnOutOfScopeVariables flag (truly turn such errors in warnings)? And what is the best place for documentation?

bgamari edited edge metadata.Aug 17 2016, 10:14 PM

Is there need for Opt_WarnOutOfScopeVariables flag (truly turn such errors in warnings)? And what is the best place for documentation?

While I do worry that the number of partial-type-error flags may be growing to be a bit large, I think we probably should include this for completeness (and perhaps enable it by default).

Regarding documentation, I think the following mentions would be helpful,

  • docs/users_guide/glasgow_exts.rst in the deter-type-errors section: Describe the default treatment of out-of-scope errors and refer to -Wout-of-scope-variables.
  • docs/users_guide/using-warnings.rst: Add a ghc-flag directive for -Wdefer-out-of-scope-variables. Make sure to explicitly mention that the flag only has an effect with -fdeter-type-errors.
  • mkUserGuidePart/Options/Warnings.hs: Define -Wdefer-out-of-scope-variables. Again, be sure to mention its scope.
simonpj requested changes to this revision.Aug 18 2016, 2:27 AM
simonpj added a reviewer: simonpj.
simonpj added a subscriber: simonpj.

You need user manual changes too!

I've suggested doing it uniformly with the other two cases,

Thanks

Simon

compiler/typecheck/TcErrors.hs
646

If we truly want this flag, can we treat it uniformly with the type holes and expression holes, by having cec_out_of_scope_holes just like cec_type_holes and cec_expr_holes. There are three things we might do: report an error, report a warning, or report nothing. That's what happens for the other two cases and we should behave the same here too, shouldn't we?

Maybe the flags don't support one of the three, but the mechanism (via cec_*) should be uniform.

This revision now requires changes to proceed.Aug 18 2016, 2:27 AM
ak3n updated this revision to Diff 8431.Aug 18 2016, 4:06 PM
ak3n edited edge metadata.
  • Add -Wdefer-out-of-scope-variables flag.
  • Add documentation for defer-out-of-scope-variables flag
ak3n updated this revision to Diff 8433.Aug 19 2016, 9:24 AM
ak3n marked an inline comment as done.
ak3n edited edge metadata.
  • Fix lint errors.
bgamari requested changes to this revision.Aug 19 2016, 1:46 PM
bgamari edited edge metadata.

Looks pretty good to me. Just one small point.

docs/users_guide/using-warnings.rst
167

Drop the "the" and perhaps hyphenate here: This will turn variable-out-of-scope errors into warnings.

This revision now requires changes to proceed.Aug 19 2016, 1:46 PM
ak3n updated this revision to Diff 8436.Aug 19 2016, 4:34 PM
ak3n edited edge metadata.
  • Fix docs.
ak3n updated this revision to Diff 8437.Aug 19 2016, 4:36 PM
ak3n edited edge metadata.
  • Fix docs.
simonpj accepted this revision.Aug 22 2016, 2:23 AM
simonpj edited edge metadata.

OK thanks. I've suggested a little more explicit documentation, but otherwise good to go.

compiler/typecheck/TcErrors.hs
293

This is a subset of "Holes in expressions". Worth a comment or Note just to say what is an "expression hole" and what is an "out of scope hole"

docs/users_guide/using-warnings.rst
164

Document the users-eye-view of how a "typed hole" differs from an "out of scope hole".

ak3n updated this revision to Diff 8455.Aug 22 2016, 2:55 PM
ak3n marked an inline comment as done.
ak3n edited edge metadata.
  • Fixes of documentation and test

Changelog entry is missing. This would go in docs/users_guide/8.2.1-notes.rst normally.

utils/mkUserGuidePart/Options/Warnings.hs
54–56

Implies :ghc-flag:-fdefer-typed-holes and -fdefer-out-of-scope-variables.

73

Typo: the warning flag is called -Wdefer-out-of-scope-variables.

For consistency with -Wdeferred-type-errors, it should probably be called -Wdeferred-out-of-scope-variables (deferred instead of defer). What do you think?

Implied by :ghc-flag:-fdefer-type-errors.

thomie requested changes to this revision.Aug 27 2016, 11:20 AM
thomie added a reviewer: thomie.
This revision now requires changes to proceed.Aug 27 2016, 11:20 AM
ak3n updated this revision to Diff 8489.Aug 27 2016, 12:16 PM
ak3n edited edge metadata.
ak3n marked 2 inline comments as done.
  • Fixes.
thomie requested changes to this revision.Aug 28 2016, 5:29 AM
thomie edited edge metadata.

Currently the name of the warning flag in the documentation and in the implementation don't match.

Please pick a name (I suggest -Wdeferred-out-of-scope-variables), and use it consistently throughout.

This revision now requires changes to proceed.Aug 28 2016, 5:29 AM
ak3n updated this revision to Diff 8501.Aug 28 2016, 9:29 AM
ak3n edited edge metadata.
ak3n marked 2 inline comments as done.
  • Change defer to deferred.
ak3n updated this revision to Diff 8502.Aug 28 2016, 9:32 AM
ak3n edited edge metadata.
  • Change defer to deferred.
ak3n added a comment.Aug 28 2016, 9:32 AM

Excuse me for my inattention.

docs/users_guide/using-warnings.rst
164

Not sure this additional info in parentheses is enough.

167

What about Defer variable out of scope errors? Variable-out-of-scope in glasgow_exts.rst?

thomie accepted this revision.Aug 28 2016, 9:54 AM
thomie edited edge metadata.

One more.

docs/users_guide/glasgow_exts.rst
9440

-Wno-deferred-out-of-scope-variables

ak3n updated this revision to Diff 8505.Aug 28 2016, 11:17 AM
ak3n marked 2 inline comments as done.
ak3n edited edge metadata.
  • Change defer to deferred.
thomie edited edge metadata.Aug 28 2016, 2:54 PM
thomie updated the Trac tickets for this revision.
bgamari accepted this revision.Aug 31 2016, 2:16 PM
bgamari edited edge metadata.

Looks good to me. Thanks @ak3n!

This revision is now accepted and ready to land.Aug 31 2016, 2:16 PM
This revision was automatically updated to reflect the committed changes.