Fix missing fields warnings in empty record construction, fix #13870
ClosedPublic

Authored by sighingnow on Sep 10 2017, 9:20 PM.

Details

Summary

Add missing fields warnings when use record construction for record-less constructors.

Test Plan

make test TEST=T13870

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.
sighingnow created this revision.Sep 10 2017, 9:20 PM

I'm newcomer to ghc and not familiar with Harbormaster, could any help me on the build failure ?

sighingnow edited the test plan for this revision. (Show Details)Sep 11 2017, 7:18 AM

Sorry for the latency; I have been travelling the last week or so.

It looks to me like your build issues may be due to the fact that you didn't use Arcanist to submit this Differential. I can update the differential with arcanist on your behalf once I get home.

sighingnow updated this revision to Diff 13840.Sep 12 2017, 7:27 PM

Fix missing fields warnings in empty record construction, fix Trac #13870.

Thanks for your hint, Ben. I have upload a new diff revision via arcanist.

sighingnow updated this revision to Diff 13841.Sep 12 2017, 7:39 PM

Fix warning message inconsistence with other test cases.

mpickering accepted this revision.Sep 13 2017, 3:21 AM
mpickering added a subscriber: mpickering.

One comment inline.

compiler/typecheck/TcExpr.hs
2426

I think it would be clearer to use when.

Why is the notNull field_strs check here?

This revision is now accepted and ready to land.Sep 13 2017, 3:21 AM

The field_strs needs to be checked because we field_strs is null, the constructor has no fields, for example, Nothing. If we write Nothing{} to construct a Maybe Int, no warning should be thrown, as described in test2 of T13870.hs.

Sorry for the typo, "we" should be "when".

sighingnow updated this revision to Diff 13842.Sep 13 2017, 3:32 AM

Use when rather than unless and not, response to @mpickering's comment.

The field_strs needs to be checked because we field_strs is null, the constructor has no fields, for example, Nothing. If we write Nothing{} to construct a Maybe Int, no warning should be thrown, as described in test2 of T13870.hs.

field_labels contains the selectors for each of the field labels. field_strs contains the strictness information for each of the fields. I was a bit vague but I don't think you need to check that both are empty, checking field_labels should be sufficient?

sighingnow marked an inline comment as done.Sep 13 2017, 7:54 AM

I didn't check that field_strs and field_labels are both empty. In checkMissingFields I checked that field_strs must NOT be empty and field_labels must be empty, and then throw the missing-fields warning.

If only field_labels is checked, when we write Nothing{}, the field_labels is empty (as well as field_strs), but this time no warning should be thrown. Indeed we don't need to provide any field to Nothing, even when use the record constructor syntax.

I didn't check that field_strs and field_labels are both empty. In checkMissingFields I checked that field_strs must NOT be empty and field_labels must be empty, and then throw the missing-fields warning.

If only field_labels is checked, when we write Nothing{}, the field_labels is empty (as well as field_strs), but this time no warning should be thrown. Indeed we don't need to provide any field to Nothing, even when use the record constructor syntax.

So if I understand you correct, you are using field_strs as a proxy for all of the fields of constructor, even those without names?

This revision was automatically updated to reflect the committed changes.