From e745a023f77de9a8cdf4135a86058922a4b63fbc Mon Sep 17 00:00:00 2001 From: Judah Jacobson Date: Mon, 26 Aug 2019 15:32:43 -0700 Subject: [PATCH 1/4] Properly implement `import public`. Fixes #19. After this change, if `bar.proto` contains the line import public foo.proto Then it turns into ``` module Proto.Bar(..., module Proto.Foo) where import Proto.Foo -- unqualified, to allow the reexport of its definitions ``` The previous hacky approach was: `Proto.Bar` doesn't reexport `Proto.Foo`; instead, if another file `baz.proto` imports `bar.proto`, *then* the codegen of `Proto.Baz` imports `Proto.Foo` as well as `Proto.Bar`. This approach let us avoid module-level exports, but had two limitations: 1) It violated the principle of "strict dependencies", since `Proto.Baz` is importing what is essentially a transitive dependency. 2) Non-codegen modules that import a proto module can't benefit from `import public` statements. --- README.md | 2 - proto-lens-protoc/app/protoc-gen-haskell.hs | 8 ++-- .../Data/ProtoLens/Compiler/Combinators.hs | 3 ++ .../src/Data/ProtoLens/Compiler/Generate.hs | 47 ++++++++++++++++--- .../src/Data/ProtoLens/Compiler/Plugin.hs | 26 +++++----- proto-lens-tests/tests/imports_test.hs | 9 ++-- 6 files changed, 66 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index beca10b2..48353b5e 100644 --- a/README.md +++ b/README.md @@ -111,8 +111,6 @@ will generate the haskell files `Proto/Project/{Foo,Bar}.hs`. - Extensions (proto2-only) are not supported. - Unknown proto2 enum values cause a decoding error, instead of being preserved round-trip. -- Files with `import public` statements compile correctly, but don't explicitly - reexport the definitions from those imports. # Troubleshooting diff --git a/proto-lens-protoc/app/protoc-gen-haskell.hs b/proto-lens-protoc/app/protoc-gen-haskell.hs index 906ac044..5ed4d0e5 100644 --- a/proto-lens-protoc/app/protoc-gen-haskell.hs +++ b/proto-lens-protoc/app/protoc-gen-haskell.hs @@ -41,6 +41,7 @@ import Data.ProtoLens.Compiler.Combinators ( prettyPrint , prettyPrintModule , getModuleName + , Module ) import Data.ProtoLens.Compiler.Generate import Data.ProtoLens.Compiler.Plugin @@ -80,14 +81,13 @@ generateFiles modifyImports header files toGenerate = let modulePrefix = "Proto" filesByName = analyzeProtoFiles modulePrefix files -- The contents of the generated Haskell file for a given .proto file. + modulesToBuild :: ProtoFile -> [Module] modulesToBuild f = let deps = descriptor f ^. dependency imports = Set.toAscList $ Set.fromList - [ haskellModule (filesByName ! exportName) - | dep <- deps - , exportName <- exports (filesByName ! dep) - ] + $ map (haskellModule . (filesByName !)) deps in generateModule (haskellModule f) imports + (publicImports f) modifyImports (definitions f) (collectEnvFromDeps deps filesByName) diff --git a/proto-lens-protoc/src/Data/ProtoLens/Compiler/Combinators.hs b/proto-lens-protoc/src/Data/ProtoLens/Compiler/Combinators.hs index cf97cc7f..9e822d52 100644 --- a/proto-lens-protoc/src/Data/ProtoLens/Compiler/Combinators.hs +++ b/proto-lens-protoc/src/Data/ProtoLens/Compiler/Combinators.hs @@ -295,6 +295,9 @@ exportWith q = Syntax.EThingWith () q . map (Syntax.ConName ()) +exportModule :: ModuleName -> ExportSpec +exportModule = Syntax.EModuleContents () + type Name = Syntax.Name () type Pat = Syntax.Pat () diff --git a/proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs b/proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs index e894c07f..f829f2af 100644 --- a/proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs +++ b/proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs @@ -60,22 +60,27 @@ data UseRuntime = UseRuntime | UseOriginal -- input contains all defined names, incl. those in this module generateModule :: ModuleName -> [ModuleName] -- ^ The imported modules + -> [ModuleName] -- ^ The publically imported modules -> ModifyImports -> Env Name -- ^ Definitions in this file -> Env QName -- ^ Definitions in the imported modules -> [ServiceInfo] -> [Module] -generateModule modName imports modifyImport definitions importedEnv services +generateModule modName imports publicImports modifyImport definitions importedEnv services = [ Module modName - (Just $ (serviceExports ++) $ concatMap generateExports $ Map.elems definitions) + (Just $ serviceExports + ++ concatMap generateExports (Map.elems definitions) + ++ map exportModule publicImports) pragmas - (mainImports ++ sharedImports) + (mainImports ++ sharedImports + ++ map importSimple (imports List.\\ publicImports) + ++ map importPublic publicImports) $ (concatMap generateDecls $ Map.toList definitions) ++ map uncommented (concatMap (generateServiceDecls env) services) , Module fieldModName Nothing pragmas - sharedImports + (sharedImports ++ map importSimple imports) . map uncommented $ concatMap generateFieldDecls allLensNames ] @@ -115,7 +120,6 @@ generateModule modName imports modifyImport definitions importedEnv services , "Data.Vector.Unboxed" , "Text.Read" ] - ++ map importSimple imports env = Map.union (unqualifyEnv definitions) importedEnv generateDecls (protoName, Message m) = generateMessageDecls fieldModName env (stripDotPrefix protoName) m @@ -143,6 +147,24 @@ allMessageFields env info = map (plainRecordField env) (messageFields info) ++ map (oneofRecordField env) (messageOneofFields info) +{- We import modules as follows: + +1) Modules from proto-lens-runtime: import qualified, strip the prefix: + import qualified Data.ProtoLens.Runtime.Data.Text as Data.Text + +2) Modules from "import" declarations: import qualified: + import qualified Proto.Foo.Bar + +3) Modules from "import public" declarations: import unqualified: + import Proto.Foo.Bar + To reexport the imported declarations from the current module via + module ... (module Proto.Foo.Bar) + the module Proto.Foo.Bar needs to be unqualified. + Alternately we could explicitly enumerate every definition being reexported, but + that would lead to less readable Haddocks and also make codegen a little more + complicated. +-} + importSimple :: ModuleName -> ImportDecl () importSimple m = ImportDecl { importAnn = () @@ -156,6 +178,19 @@ importSimple m = ImportDecl , importSpecs = Nothing } +importPublic :: ModuleName -> ImportDecl () +importPublic m = ImportDecl + { importAnn = () + , importModule = m + -- Don't import qualified so that this module can reexport its definitions. + , importQualified = False + , importSrc = False + , importSafe = False + , importPkg = Nothing + , importAs = Nothing + , importSpecs = Nothing + } + type ModifyImports = ImportDecl () -> ImportDecl () reexported :: ModifyImports @@ -967,7 +1002,7 @@ fieldAccessorExpr (PlainFieldInfo kind f) = accessorCon @@ fieldOfExp hsFieldNam -> "Data.ProtoLens.MapField" @@ fieldOfExp (overloadedField $ keyField entry) @@ fieldOfExp (overloadedField $ valueField entry) - RepeatedField packed -> + RepeatedField packed -> "Data.ProtoLens.RepeatedField" @@ if packed == Packed then "Data.ProtoLens.Packed" diff --git a/proto-lens-protoc/src/Data/ProtoLens/Compiler/Plugin.hs b/proto-lens-protoc/src/Data/ProtoLens/Compiler/Plugin.hs index 69e15a70..7568c42f 100644 --- a/proto-lens-protoc/src/Data/ProtoLens/Compiler/Plugin.hs +++ b/proto-lens-protoc/src/Data/ProtoLens/Compiler/Plugin.hs @@ -42,10 +42,8 @@ data ProtoFile = ProtoFile , haskellModule :: ModuleName , definitions :: Env Name , services :: [ServiceInfo] - -- | The names of proto files exported (transitively, via "import public" - -- decl) by this file. - , exports :: [ProtoFileName] , exportedEnv :: Env QName + , publicImports :: [ModuleName] } -- Given a list of FileDescriptorProtos, collect information about each file @@ -59,23 +57,24 @@ analyzeProtoFiles modulePrefix files = -- The definitions in each input proto file, indexed by filename. definitionsByName = fmap collectDefinitions filesByName servicesByName = fmap collectServices filesByName - -- The exports from each .proto file (including any "public import" - -- dependencies), as they appear to other modules that are importing them; - -- i.e., qualified by module name. exportsByName = transitiveExports files - localExports = Map.intersectionWith qualifyEnv moduleNames definitionsByName - exportedEnvs = fmap (\es -> unions [localExports ! e | e <- es]) exportsByName + exportedEnvs = fmap (foldMap (definitionsByName !)) exportsByName ingestFile f = ProtoFile { descriptor = f - , haskellModule = moduleNames ! n + , haskellModule = m , definitions = definitionsByName ! n , services = servicesByName ! n - , exports = exportsByName ! n - , exportedEnv = exportedEnvs ! n + , exportedEnv = qualifyEnv m $ exportedEnvs ! n + , publicImports = [moduleNames ! i | i <- reexported] } where n = f ^. name + m = moduleNames ! n + reexported = + [ (f ^. dependency) !! fromIntegral i + | i <- f ^. publicDependency + ] collectEnvFromDeps :: [ProtoFileName] -> Map ProtoFileName ProtoFile -> Env QName collectEnvFromDeps deps filesByName = @@ -106,11 +105,13 @@ moduleNameStr prefix path = fixModuleName rawModuleName . splitDirectories $ dropExtension $ path + -- | Given a list of .proto files (topologically sorted), determine which -- files' definitions are exported by which files. -- -- Files only export their own definitions, along with the definitions exported --- by any "import public" declarations. +-- by any "import public" declarations. (And any definitions that *those* files +-- "import public", etc.) transitiveExports :: [FileDescriptorProto] -> Map ProtoFileName [ProtoFileName] -- Accumulate the transitive dependencies by folding over the files in -- topological order. @@ -127,4 +128,3 @@ transitiveExports = foldl' setExportsFromFile Map.empty | i <- fd ^. publicDependency ] where n = fd ^. name - diff --git a/proto-lens-tests/tests/imports_test.hs b/proto-lens-tests/tests/imports_test.hs index 564e1769..50f95803 100644 --- a/proto-lens-tests/tests/imports_test.hs +++ b/proto-lens-tests/tests/imports_test.hs @@ -15,8 +15,6 @@ import qualified Proto.Enum as Enum import qualified Proto.Imports as Imports import qualified Proto.ImportsDep as ImportsDep import qualified Proto.Nested as Nested -import qualified Proto.ImportsTransitive as ImportsTransitive -import qualified Proto.ImportsTransitive2 as ImportsTransitive2 import qualified Proto.Google.Protobuf.Compiler.Plugin as Plugin import qualified Proto.Google.Protobuf.Descriptor as Descriptor @@ -46,9 +44,12 @@ testFoo = testCase "testFoo" $ do testUseDep :: Test testUseDep = testCase "testUseDep" $ do + -- Due to "import public" statements, imports_dep.proto reexports the + -- modules in both imports_transitive.proto and (transitively) + -- imports_transitive2.proto. testField @Imports.UseDep @ImportsDep.DepPkg #foo - testField @Imports.UseDep @ImportsTransitive.TransitiveDep #bar - testField @Imports.UseDep @ImportsTransitive2.TransitiveDep2 #baz + testField @Imports.UseDep @ImportsDep.TransitiveDep #bar + testField @Imports.UseDep @ImportsDep.TransitiveDep2 #baz testUseBootstrapped :: Test testUseBootstrapped = testCase "testUseBootstrapped" $ do From 66ede81c0016a175b516b735f79d1d6fc2307826 Mon Sep 17 00:00:00 2001 From: Judah Jacobson Date: Tue, 27 Aug 2019 11:35:29 -0700 Subject: [PATCH 2/4] Update the changelog. --- proto-lens-protoc/Changelog.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/proto-lens-protoc/Changelog.md b/proto-lens-protoc/Changelog.md index 75cb1af3..cd26f697 100644 --- a/proto-lens-protoc/Changelog.md +++ b/proto-lens-protoc/Changelog.md @@ -3,6 +3,8 @@ ## Pending - Fix a potential naming conflict when message types and enum values are the same except for case. +- Reexport transitive definitions from files with `import public` statements + (#329). ## v0.5.0.0 From 75effda6eff909b826ccba9a08a54733066ad35c Mon Sep 17 00:00:00 2001 From: Judah Jacobson Date: Tue, 27 Aug 2019 21:20:05 -0700 Subject: [PATCH 3/4] Add back old test cases --- proto-lens-tests/tests/imports_test.hs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/proto-lens-tests/tests/imports_test.hs b/proto-lens-tests/tests/imports_test.hs index 50f95803..a1bc2523 100644 --- a/proto-lens-tests/tests/imports_test.hs +++ b/proto-lens-tests/tests/imports_test.hs @@ -15,6 +15,8 @@ import qualified Proto.Enum as Enum import qualified Proto.Imports as Imports import qualified Proto.ImportsDep as ImportsDep import qualified Proto.Nested as Nested +import qualified Proto.ImportsTransitive as ImportsTransitive +import qualified Proto.ImportsTransitive2 as ImportsTransitive2 import qualified Proto.Google.Protobuf.Compiler.Plugin as Plugin import qualified Proto.Google.Protobuf.Descriptor as Descriptor @@ -44,12 +46,14 @@ testFoo = testCase "testFoo" $ do testUseDep :: Test testUseDep = testCase "testUseDep" $ do - -- Due to "import public" statements, imports_dep.proto reexports the - -- modules in both imports_transitive.proto and (transitively) - -- imports_transitive2.proto. testField @Imports.UseDep @ImportsDep.DepPkg #foo + testField @Imports.UseDep @ImportsTransitive.TransitiveDep #bar + testField @Imports.UseDep @ImportsTransitive2.TransitiveDep2 #baz + -- Due to "import public" statements, these modules reexport their + -- dependencies transitively: testField @Imports.UseDep @ImportsDep.TransitiveDep #bar testField @Imports.UseDep @ImportsDep.TransitiveDep2 #baz + testField @Imports.UseDep @ImportsTransitive.TransitiveDep2 #baz testUseBootstrapped :: Test testUseBootstrapped = testCase "testUseBootstrapped" $ do From d6b0ffa308768c5e46cc046269f31a39c61ee7e0 Mon Sep 17 00:00:00 2001 From: Judah Jacobson Date: Tue, 27 Aug 2019 21:25:09 -0700 Subject: [PATCH 4/4] Split out breaking and nonbreaking changes in readme --- proto-lens-protoc/Changelog.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/proto-lens-protoc/Changelog.md b/proto-lens-protoc/Changelog.md index cd26f697..492911fc 100644 --- a/proto-lens-protoc/Changelog.md +++ b/proto-lens-protoc/Changelog.md @@ -1,10 +1,14 @@ # Changelog for `proto-lens-protoc` ## Pending + +### Breaking Changes +- Reexport transitive definitions from modules generated for `.proto` files + with `import public` statements (#329). + +### Backwards-Compatible Changes - Fix a potential naming conflict when message types and enum values are the same except for case. -- Reexport transitive definitions from files with `import public` statements - (#329). ## v0.5.0.0