Ensure proper case-less handling of FlagName by peti · Pull Request #5082 · haskell/cabal

peti · 2018-01-31T11:52:52Z

This patch moves the conversion to all-lower-case out of the FlagName parser instances and into the mkFlagName constructor function.

Fixes #5043.

Please include the following checklist in your PR:

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
The documentation has been updated, if necessary.
If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

23Skidoo · 2018-01-31T20:17:40Z

There are some legit CI failures: it fails to build on older GHCs and AppVeyor found this:

Unit Tests
  Distribution.Types.GenericPackageDescription
    findDuplicateFlagAssignments:                                                         FAIL
      tests\UnitTests\Distribution\Types\GenericPackageDescription.hs:53:
      unexpected: []
    FlagName is case-insensitive:                                                         FAIL
      *** Failed! Falsifiable (after 26 tests):
      [(FlagName "B",True),(FlagName "Vp",True),(FlagName "T",False),(FlagName "Y",False),(FlagName "u",True),(FlagName "We",False),(FlagName "I",True),(FlagName "Y",True),(FlagName "m",False),(FlagName "o",False),(FlagName "D",False),(FlagName "C",False),(FlagName "f",False),(FlagName "d",True),(FlagName "Tv",False),(FlagName "l",True),(FlagName "dM",False),(FlagName "g",False),(FlagName "sc",False),(FlagName "M",False),(FlagName "DS",False),(FlagName "aE",True),(FlagName "ea",True),(FlagName "W",False),(FlagName "df",False)]
      Use --quickcheck-replay=474123 to reproduce.

phadej · 2018-01-31T20:26:12Z

+    let fa'        = nub [ map toLower (unFlagName n) | (n,_) <- fa ]
+        duplicates = findDuplicateFlagAssignments (mkFlagAssignment fa)
+    in
+       length duplicates == length fa - length fa'


use === (triple equals) to print LHS and RHS in case of failure

@phadej, I force-pushed a new version (72e4f55) that uses ===.

peti · 2018-01-31T21:01:25Z

@23Skidoo, yes, of course! That failure documents the fact that our current API does not ensure that flag name are treated in a case-insensitive way. We apply lowercase when parsing a FlagAssignment from String, but any other way of creating that type bypasses this conversion and then foo and FOO are, in fact, two distinct flag names. These tests should succeed, but they don't because our implementation is, arguably, incorrect.

23Skidoo · 2018-01-31T21:09:31Z

Do you want to mark it as expected failure (to be fixed later) and merge as is, or do you plan to fix the underlying issue as part of the same PR?

peti · 2018-01-31T21:12:41Z

@23Skidoo, I would like to fix the issue in this PR. The obvious solution would be to move the conversion to all lower-case into the mkFlagName function. I suggested that in #5043. So far, nobody has spoken out against that change, but curiously enough no-one has spoken out in favor of it either, so I haven't made the change yet. I probably should.

23Skidoo · 2018-01-31T21:15:11Z

Noted.

peti · 2018-02-06T16:20:39Z

OK, I fixed the underlying issue and moved the map toLower conversion out of the FlagName parser code and into the mkFlagName constructor function. I also fixed the build errors with older compilers and re-based my patch to the current master.

There are still CI failures in Travis et al, but these failures seem to be unrelated to my changes.

peti · 2018-02-12T17:37:51Z

All CI tests on travis are green. I think this PR is ready to be merged.

23Skidoo · 2018-02-13T13:07:05Z

There are some failing tests in downstream CI builds.

This commit adds new unit tests to the Cabal test suite to verify that 'findDuplicateFlagAssignments' recognizes properly cases in which a 'FlagAssignment' was constructed from a list that contains flag names "foo" and "FOO" -- which ought to be a duplicate. Related to #5043.

…ag names

peti · 2018-03-23T17:35:16Z

I don't know what is causing these CI test failures in appveyor. I have run the cabal test suite locally and it succeeds just fine on my machine.

hvr · 2018-03-23T17:55:00Z

Well, there's still the issue that this PR introduces a technical debt we'll have to revert when cleaning up the semantics as e.g. suggested in #5043 (comment), as then we can't just normalise in mkFlagName; instead we'll have to move it out again, effectively reverting large parts of this PR

peti · 2018-03-23T18:18:50Z

Well, there is also the issue that mkFlagName allows users to create a FlagName that violates the semantics of that type, and that problem exists today, whereas those new-and-improved semantics are going to exist at some point in the future of Cabal 3.0, maybe.

peti · 2018-04-16T13:47:31Z

So ... I trust this all means that this PR will be lying here forever and that I might just as well close it?

23Skidoo · 2018-04-23T17:23:50Z

@hvr @phadej Would you be ok with accepting this PR if lowercase was replaced with ifM (all isAscii) lowercase id?

23Skidoo · 2018-04-24T08:24:16Z

@phadej @hvr Ping ^.

peti mentioned this pull request Jan 31, 2018

Why does mkFlagName not convert the string to lower-case? #5043

Closed

23Skidoo added the blocked: action-required label Jan 31, 2018

phadej reviewed Jan 31, 2018

View reviewed changes

peti changed the title ~~Cabal: extend unit test suite to verify case-less handling of FlagName~~ Ensure proper case-less handling of FlagName Feb 12, 2018

peti added attention: needs-review and removed blocked: action-required labels Feb 12, 2018

23Skidoo added the blocked: action-required label Feb 14, 2018

peti added 2 commits March 23, 2018 16:52

cabal-install: adapt solver tests to expect the new all lower-case fl…

12a19e8

…ag names

peti added type: enhancement and removed blocked: action-required labels Mar 23, 2018

peti closed this Apr 23, 2018

Conversation

peti commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

23Skidoo commented Jan 31, 2018

Uh oh!

phadej Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peti Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

peti commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

23Skidoo commented Jan 31, 2018

Uh oh!

peti commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

23Skidoo commented Jan 31, 2018

Uh oh!

peti commented Feb 6, 2018

Uh oh!

peti commented Feb 12, 2018

Uh oh!

23Skidoo commented Feb 13, 2018

Uh oh!

peti commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hvr commented Mar 23, 2018

Uh oh!

peti commented Mar 23, 2018

Uh oh!

peti commented Apr 16, 2018

Uh oh!

23Skidoo commented Apr 23, 2018

Uh oh!

23Skidoo commented Apr 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

peti commented Jan 31, 2018 •

edited

Loading

phadej Jan 31, 2018 •

edited

Loading

peti commented Jan 31, 2018 •

edited

Loading

peti commented Jan 31, 2018 •

edited

Loading

peti commented Mar 23, 2018 •

edited

Loading