Use UnqualComponentName newtype instead of String for component names #4057

Ericson2314 · 2016-10-29T08:06:45Z

Working on #4055 led into this... 🎊

mention-bot · 2016-10-29T08:06:47Z

@Ericson2314, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ezyang, @dcoutts and @edsko to be potential reviewers.

Ericson2314 · 2016-10-29T08:09:28Z

Hmm this bitrots real quick

dcoutts · 2016-10-30T17:14:05Z

Hmm this bitrots real quick

You were just unlucky, ezyang merged the huge foreign libraries patch. Those don't come along very often.

dcoutts · 2016-10-30T17:16:13Z

So I'm lacking some context. Why are we renaming component name to unqualified component name?

ezyang · 2016-10-30T17:21:08Z

We're not. ComponentName is still a library/executable/... tag + a string, and UnqualComponentName is just the string without the tag, which shows up in a number of places in Cabal source code. It's more type safe.

ezyang · 2016-10-31T09:19:48Z

Appveyor failure is real. I guess some Windows specific code that is macro'd away?

ezyang

Looks good. But the Appveyor build failure needs to be fixed. The rest of the comments can wait after merging, to help avoid bitrotting.

ezyang · 2016-10-31T09:20:54Z

Cabal/Distribution/Backpack/ConfiguredComponent.hs

@@ -183,7 +183,8 @@ extendConfiguredComponentMap cc (lib_map, exe_map) =
            Map.insert (pkgName (cc_pkgid cc))
                       (cc_cid cc, cc_pkgid cc) lib_map
          CSubLibName str ->
-            Map.insert (mkPackageName str)
+            -- TODO roundtrip
+            Map.insert (mkPackageName $ unUnqualComponentName str)


Didn't you make a function to handle this conversion?

ezyang · 2016-10-31T09:21:33Z

Cabal/Distribution/Package.hs

@@ -69,6 +70,44 @@ import Distribution.ModuleName

 import Text.PrettyPrint ((<+>), text)

+-- | A Component name, or other similarly-parsed identifier.


Distinguished from a 'ComponentName', which also identifies whether or not the component in question is a library, executable, etc.

ezyang · 2016-10-31T09:22:49Z

Cabal/Distribution/Package.hs

+instance Binary UnqualComponentName
+
+instance Text UnqualComponentName where
+  disp = Disp.text . unUnqualComponentName


Explanation that this was cribbed from PackageName would be nice.

ezyang · 2016-10-31T09:23:51Z

Cabal/Distribution/Package.hs

+    deriving (Generic, Read, Show, Eq, Ord, Typeable, Data,
+              Binary, Text, NFData)
+
+packageNameToUnqualComponentName :: PackageName -> UnqualComponentName


Needs doc. Something like "Converts a package name to an unqualified component name; useful in legacy situations where a package name may refer to an internal component, if one is defined with that name."

ezyang · 2016-10-31T09:25:04Z

Cabal/Distribution/Package.hs

@@ -77,12 +116,16 @@ import Text.PrettyPrint ((<+>), text)
 -- This type is opaque since @Cabal-2.0@
 --
 -- @since 2.0
-newtype PackageName = PackageName ShortText
-    deriving (Generic, Read, Show, Eq, Ord, Typeable, Data)
+newtype PackageName = PackageName UnqualComponentName


I'm.... not sure setting it up this way makes sense? A package name isn't really a component name in any sense; they just happen to coincide in syntax due to some historical choices. Better to keep them disjoint? (If the duplication of the Text instance is troublesome, define a helper function.)

ezyang · 2016-10-31T09:25:46Z

Cabal/Distribution/PackageDescription/Check.hs

@@ -185,13 +185,15 @@ checkSanity pkg =
        ++ "Only the non-internal library can have the same name as the package."

  , check (not (null duplicateNames)) $
-      PackageBuildImpossible $ "Duplicate sections: " ++ commaSep duplicateNames
+      PackageBuildImpossible $ "Duplicate sections: "
+        ++ commaSep (unUnqualComponentName <$> duplicateNames)


Can't say I'm the biggest fan of fmap here, when a map makes intent much clearer ;)

ezyang · 2016-10-31T09:26:11Z

Cabal/Distribution/PackageDescription/Parse.hs

+                  ,[(UnqualComponentName, CondTree ConfVar [Dependency] ForeignLib)]
+                  ,[(UnqualComponentName, CondTree ConfVar [Dependency] Executable)]
+                  ,[(UnqualComponentName, CondTree ConfVar [Dependency] TestSuite)]
+                  ,[(UnqualComponentName, CondTree ConfVar [Dependency] Benchmark)])


Ah, someday we'll refactor this code. Not today though :)

Delete I hope! Its the old parser.

Ericson2314 · 2016-10-31T13:22:05Z

No windows error seems spurious?

ezyang · 2016-10-31T18:19:27Z

Yeah. For some reason Appveyor isn't letting me request a rebuild. Maybe rebase and force push to kick the build again.

Ericson2314 · 2016-10-31T19:22:17Z

Hmm, for future reference it would be nice to avoid repeating the signatures of symlinkBinar{y,ies}, as only their implementations are platform-specific.

ezyang · 2016-10-31T20:05:26Z

Yes! My preference is to minimize the amount of code covered by CPP but there is a lot of code which doesn't follow this rule.

Ericson2314 · 2016-11-01T02:56:06Z

Whew both passed! Cause this bitrots so quick I'm going to merge. I hope that's ok!

ezyang · 2016-11-01T03:59:16Z

Thank you, good work!

Ericson2314 · 2017-01-18T15:31:46Z

I need to remember to move the WIP before the merge commit, lol!

phadej · 2017-01-18T16:52:58Z

Cabal/Distribution/PackageDescription/Parsec.hs

@@ -363,6 +364,10 @@ parseName pos args = case args of
         parseFailure pos $ "Invalid name " ++ show args
         pure ""

+parseUnqualComponentName :: Position -> [SectionArg Position] -> ParseResult UnqualComponentName
+parseUnqualComponentName pos args = mkUnqualComponentName <$> parseName pos args


I wonder if parseName is used anywhere else, otherwise we could inline it here.

You mean not expose it? It is used a couple time in that module if I am reading this right and it wasn't changed since this was merged.

Ah, it used multiple times. Then everything is ok, sorry for disturbance.

No worries! I think it was being weird about rendering the diff.

Ericson2314 force-pushed the unqual-component-name branch from 761abbf to 2cd5ac2 Compare October 30, 2016 00:29

Ericson2314 force-pushed the unqual-component-name branch 3 times, most recently from b93ccd3 to b008fcd Compare October 31, 2016 03:48

Ericson2314 mentioned this pull request Oct 31, 2016

Make more dependency types #4067

Merged

ezyang requested changes Oct 31, 2016

View reviewed changes

Ericson2314 force-pushed the unqual-component-name branch 2 times, most recently from 8271b2d to ac8ad4a Compare October 31, 2016 13:01

ezyang approved these changes Oct 31, 2016

View reviewed changes

Ericson2314 force-pushed the unqual-component-name branch from ac8ad4a to 63502b8 Compare October 31, 2016 18:38

Use new UnqualComponentName newtype instead of String

be6a94a

Ericson2314 force-pushed the unqual-component-name branch from 63502b8 to be6a94a Compare October 31, 2016 19:16

Ericson2314 changed the title ~~[WIP] Use UnqualComponentName instead of String for component names~~ [WIP] Use UnqualComponentName newtype instead of String for component names Oct 31, 2016

Ericson2314 merged commit acc460d into haskell:master Nov 1, 2016

ezyang added this to the 2.0 milestone Nov 1, 2016

ezyang assigned Ericson2314 Nov 1, 2016

Ericson2314 deleted the unqual-component-name branch November 1, 2016 13:07

Ericson2314 changed the title ~~[WIP] Use UnqualComponentName newtype instead of String for component names~~ Use UnqualComponentName newtype instead of String for component names Jan 18, 2017

phadej reviewed Jan 18, 2017

View reviewed changes

		@@ -69,6 +70,44 @@ import Distribution.ModuleName

		import Text.PrettyPrint ((<+>), text)

		-- \| A Component name, or other similarly-parsed identifier.

Use UnqualComponentName newtype instead of String for component names #4057

Use UnqualComponentName newtype instead of String for component names #4057

Uh oh!

Conversation

Ericson2314 commented Oct 29, 2016

Uh oh!

mention-bot commented Oct 29, 2016

Uh oh!

Ericson2314 commented Oct 29, 2016

Uh oh!

dcoutts commented Oct 30, 2016

Uh oh!

dcoutts commented Oct 30, 2016

Uh oh!

ezyang commented Oct 30, 2016

Uh oh!

ezyang commented Oct 31, 2016

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Oct 31, 2016

Uh oh!

ezyang commented Oct 31, 2016

Uh oh!

Ericson2314 commented Oct 31, 2016

Uh oh!

ezyang commented Oct 31, 2016

Uh oh!

Ericson2314 commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ezyang commented Nov 1, 2016

Uh oh!

Ericson2314 commented Jan 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ericson2314 commented Nov 1, 2016 •

edited

Loading