Fix "redundant constraint" warnings when meets functional dependencies
AbandonedPublic

Authored by sighingnow on May 26 2018, 9:16 AM.

Details

Reviewers
bgamari
Trac Issues
#11474
Summary

When a class constraint itself or it's superclasses have functional dependencies, don't emit redundant-constraints warnings. Fix Trac Trac #11474.

Test Plan

make test TEST="T10100 T11474"

sighingnow created this revision.May 26 2018, 9:16 AM
sighingnow retitled this revision from When a class constraint itself or it's superclasses have functional dependencies, don't emit redundant-constraints warnings. Fix Trac #11474. to Fix "redundant constraint" warnings when meets functional dependencies.May 26 2018, 9:18 AM
sighingnow edited the summary of this revision. (Show Details)
dfeuer added a subscriber: dfeuer.May 27 2018, 6:26 PM
dfeuer added inline comments.
compiler/typecheck/TcErrors.hs
475

It would be great to add a brief comment explaining what this is doing. For example, why is there now a case match checking ClassPred? Also, why did you shift the idType out of improving?

testsuite/tests/typecheck/should_compile/T11474.hs
5

While not mandatory, a brief comment about what this is testing would be helpful.

sighingnow updated this revision to Diff 16549.May 27 2018, 9:32 PM
  • Add some comments.
sighingnow marked 2 inline comments as done.May 27 2018, 9:32 PM
sighingnow added inline comments.May 27 2018, 9:36 PM
compiler/typecheck/TcErrors.hs
475

Also, why did you shift the idType out of improving?

If don't, then improving will be:

improving ev_ty =
   case classifyPredType (idType ev_ty) of
     ClassPred _ _ -> isImprovementPred (idType ev_ty) ||
                      any isImprovementPred (transSuperClasses (idType ev_ty))
     _             -> False

I thinks these idTypes look a bit of redundant.

tdammers added inline comments.
compiler/typecheck/TcErrors.hs
475

A few minor nitpicks about spelling, grammar, and style. How about:

-- When a constraint is a typeclass constraint, and the typeclass itself or any
-- of its superclasses have functional dependencies, we won't consider such
-- constraints redundant.
-- See Note [Redundant constraints in instance decls]
sighingnow updated this revision to Diff 16787.Jun 8 2018, 3:43 AM
  • Add some comments.
  • Revise comments and fix "Too long lines" lint warnings.
sighingnow marked 2 inline comments as done.Jun 8 2018, 3:43 AM
sighingnow updated this revision to Diff 16788.Jun 8 2018, 4:26 AM
  • Fix compile error.
bgamari added inline comments.Jun 17 2018, 11:40 AM
compiler/typecheck/TcErrors.hs
475

I'll admit I'm still not sure I see why the classifyPredType is now needed. How will fundeps manifest here?

sighingnow added inline comments.Jun 17 2018, 11:11 PM
compiler/typecheck/TcErrors.hs
475

Previously, we only consider whether any of it's superclasses have functional dependencies, without the typeclass constraint itself.

Now we need to consider whether the typeclass itself has functional dependencies as well. However the isImprovementPred also treats some other constraints as "isImprovement":

isImprovementPred :: PredType -> Bool
-- Either it's an equality, or has some functional dependency
isImprovementPred ty
  = case classifyPredType ty of
      EqPred NomEq t1 t2 -> not (t1 `tcEqType` t2)
      EqPred ReprEq _ _  -> False
      ClassPred cls _    -> classHasFds cls
      IrredPred {}       -> True -- Might have equalities after reduction?
      ForAllPred {}      -> False

Thus I think classifyPredType is now needed.

  • Add some comments.
  • Revise comments and fix "Too long lines" lint warnings.
  • Fix compile error.
  • Replace * with Type in T11474.
sighingnow marked an inline comment as done.Jun 22 2018, 9:47 AM

The only CI failures are on OS X and they're stats failures. I'm no typechecker expert but the feedback got addressed and the test included with this patch is indeed passing. Any objection to merging this fix?