Hyphenated file support

compile/compiler.go

@@ -22,6 +22,7 @@ package compile
 
 import (
 	"path/filepath"
+	"strings"
 
 	"go.uber.org/thriftrw/ast"
 	"go.uber.org/thriftrw/idl"
@@ -242,6 +243,7 @@ func (c compiler) gather(m *Module, prog *ast.Program) error {
 // include loads the file specified by the given include in the given Module.
 //
 // The path to the file is relative to the ThriftPath of the given module.
+// Including hyphenated file names will error.
 func (c compiler) include(m *Module, include *ast.Include) (*IncludedModule, error) {
 	if len(include.Name) > 0 {
 		// TODO(abg): Add support for include-as flag somewhere.
@@ -251,6 +253,13 @@ func (c compiler) include(m *Module, include *ast.Include) (*IncludedModule, err
 		}
 	}
 
+	if strings.Contains(fileBaseName(include.Path), "-") {
+		return nil, includeError{
+			Include: include,
+			Reason:  includeHyphenatedFileNameError{},
+		}
+	}
+
 	ipath := filepath.Join(filepath.Dir(m.ThriftPath), include.Path)
 	incM, err := c.load(ipath)
 	if err != nil {

compile/error.go

@@ -64,6 +64,14 @@ func (e includeAsDisabledError) Error() string {
 	return "include-as syntax is currently disabled"
 }
 
+// includeHyphenatedFileNameError is raised when the user attempts to
+// include hyphenated file names.
+type includeHyphenatedFileNameError struct{}
+
+func (e includeHyphenatedFileNameError) Error() string {
+	return "cannot include hyphenated Thrift files"
+}
+
 // includeError is raised when there is an error including another Thrift
 // file.
 type includeError struct {

gen/generate.go

@@ -165,6 +165,36 @@ func Generate(m *compile.Module, o *Options) error {
 	return nil
 }
 
+func duplicateAfterNormalization(fileName string) error {
+	absFileName, err := filepath.Abs(fileName)
+	if err != nil {
+		return generateError{
+			Name:   fileName,
+			Reason: fmt.Errorf("File %q does not exist: %v", fileName, err),
+		}
+	}
+	normalizedFileName := normalizeFilePath(absFileName)
+	fileNormalized := absFileName != normalizedFileName
+	if fileNormalized && fileExists(normalizedFileName) {
+		return fmt.Errorf("normalized file %q collides with existing file %q", normalizedFileName, absFileName)
+	}
+	return nil
+}
+
+func fileExists(filename string) bool {
+	info, err := os.Stat(filename)
+	if os.IsNotExist(err) {
+		return false
+	}
+	return !info.IsDir()
+}
+
+// normalizeFilePath replaces hyphens in the file name with underscores.
+func normalizeFilePath(p string) string {
+	fileName := strings.Replace(filepath.Base(p), "-", "_", -1)
+	return filepath.Join(filepath.Dir(p), fileName)
+}
+
 // ThriftPackageImporter determines import paths from a Thrift root.
 type ThriftPackageImporter interface {
 	// RelativePackage returns the import path for the top-level package of the
@@ -186,7 +216,7 @@ type thriftPackageImporter struct {
 }
 
 func (i thriftPackageImporter) RelativePackage(file string) (string, error) {
-	return filepath.Rel(i.ThriftRoot, strings.TrimSuffix(file, ".thrift"))
+	return filepath.Rel(i.ThriftRoot, strings.TrimSuffix(normalizeFilePath(file), ".thrift"))
 }
 
 func (i thriftPackageImporter) RelativeThriftFilePath(file string) (string, error) {
@@ -226,12 +256,17 @@ func generateModule(
 	builder *generateServiceBuilder,
 	o *Options,
 ) (outputFilepath string, contents []byte, err error) {
+	if err := duplicateAfterNormalization(m.ThriftPath); err != nil {
+		return "", nil, err
+	}
+	// converts file from /home/abc/ab-def.thrift to /home/abc/ab_def.thrift for golang code generation
+	normalizedThriftPath := normalizeFilePath(m.ThriftPath)
 	// packageRelPath is the path relative to outputDir into which we'll be
 	// writing the package for this Thrift file. For $thriftRoot/foo/bar.thrift,
 	// packageRelPath is foo/bar, and packageDir is $outputDir/foo/bar. All
 	// files for bar.thrift will be written to the $outputDir/foo/bar/ tree. The
 	// package will be importable via $importPrefix/foo/bar.
-	packageRelPath, err := i.RelativePackage(m.ThriftPath)
+	packageRelPath, err := i.RelativePackage(normalizedThriftPath)
 	if err != nil {
 		return "", nil, err
 	}

gen/generate_test.go

@@ -79,6 +79,67 @@ func TestGenerateWithRelativePaths(t *testing.T) {
 	}
 }
 
+func TestGenerateWithHyphenPaths(t *testing.T) {
+	outputDir, err := ioutil.TempDir("", "thriftrw-generate-test")
+	require.NoError(t, err)
+	defer os.RemoveAll(outputDir)
+
+	thriftRoot, err := os.Getwd()
+	require.NoError(t, err)
+
+	tests := []struct {
+		name          string
+		filepath      string
+		generateError string
+		compileError  string
+	}{
+		{
+			name:         "include with hyphens error",
+			filepath:     "internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift",
+			compileError: "cannot include hyphenated Thrift files",
+		},
+		{
+			name:     "hyphen files as top level file allowed",
+			filepath: "internal/tests/thrift/hyphenated-file.thrift",
+		},
+		{
+			name:          "normalization collision err",
+			filepath:      "internal/tests/thrift/nestedfiles_error/hyphenated-file-nested.thrift",
+			generateError: "collides with existing file",
+		},
+		{
+			name:     "normalization success",
+			filepath: "internal/tests/thrift/nestedfiles_error/hyphenated_file_nested.thrift",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			module, err := compile.Compile(tt.filepath)
+			if tt.compileError != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tt.compileError)
+				return
+			}
+			require.NoError(t, err)
+			require.NotNil(t, module)
+
+			opt := &Options{
+				OutputDir:     outputDir,
+				PackagePrefix: "go.uber.org/thriftrw/gen",
+				ThriftRoot:    thriftRoot,
+			}
+			err = Generate(module, opt)
+			if tt.generateError != "" {
+				require.Error(t, err, "expected code generation with filepath %v to fail", tt.filepath)
+				assert.Contains(t, err.Error(), tt.generateError)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
 func TestGenerate(t *testing.T) {
 	var (
 		ts compile.TypeSpec = &compile.TypedefSpec{

gen/golden_test.go

@@ -58,7 +58,7 @@ func TestCodeIsUpToDate(t *testing.T) {
 	defer os.RemoveAll(outputDir)
 
 	for _, thriftFile := range thriftFiles {
-		pkgRelPath := strings.TrimSuffix(filepath.Base(thriftFile), ".thrift")
+		pkgRelPath := strings.TrimSuffix(normalizeFilePath(filepath.Base(thriftFile)), ".thrift")
 		currentPackageDir := filepath.Join("internal/tests", pkgRelPath)
 		newPackageDir := filepath.Join(outputDir, pkgRelPath)
 

gen/internal/tests/Makefile

@@ -18,4 +18,4 @@ nozap: thrift/nozap.thrift $(THRIFTRW)
 	$(THRIFTRW) --no-recurse --no-zap $<
 
 %: thrift/%.thrift $(THRIFTRW)
-	$(THRIFTRW) --no-recurse $<
+	$(THRIFTRW) $<