Skip to content

Commit

Permalink
fix #2177: Revert "check for consistent path metadata from plugins"
Browse files Browse the repository at this point in the history
This reverts commit 8534ad3.
This reverts commit efbf69c.
This reverts commit ddd53f9.
  • Loading branch information
evanw committed Apr 12, 2022
1 parent 757d7d9 commit a6d80fc
Show file tree
Hide file tree
Showing 7 changed files with 23 additions and 285 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## Unreleased

* Revert path metadata validation for now ([#2177](https://github.com/evanw/esbuild/issues/2177))

This release reverts the path metadata validation that was introduced in the previous release. This validation has uncovered a potential issue with how esbuild handles `onResolve` callbacks in plugins that will need to be fixed before path metadata validation is re-enabled.

## 0.14.35

* Add support for parsing `typeof` on #private fields from TypeScript 4.7 ([#2174](https://github.com/evanw/esbuild/pull/2174))
Expand Down
75 changes: 17 additions & 58 deletions internal/bundler/bundler.go
Original file line number Diff line number Diff line change
Expand Up @@ -788,7 +788,6 @@ func RunOnResolvePlugins(
return &resolver.ResolveResult{
PathPair: resolver.PathPair{Primary: result.Path},
IsExternal: result.External,
PluginName: pluginName,
PluginData: result.PluginData,
PrimarySideEffectsData: sideEffectsData,
}, false, resolver.DebugMeta{}
Expand Down Expand Up @@ -969,6 +968,14 @@ func loaderFromFileExtension(extensionToLoader map[string]config.Loader, base st
return config.LoaderNone
}

// Identify the path by its lowercase absolute path name with Windows-specific
// slashes substituted for standard slashes. This should hopefully avoid path
// issues on Windows where multiple different paths can refer to the same
// underlying file.
func canonicalFileSystemPathForWindows(absPath string) string {
return strings.ReplaceAll(strings.ToLower(absPath), "\\", "/")
}

func hashForFileName(hashBytes []byte) string {
return base32.StdEncoding.EncodeToString(hashBytes)[:8]
}
Expand All @@ -995,10 +1002,7 @@ type scanner struct {
}

type visitedFile struct {
importSource *logger.Source
resolveResult resolver.ResolveResult
importPathRange logger.Range
sourceIndex uint32
sourceIndex uint32
}

type EntryPoint struct {
Expand Down Expand Up @@ -1152,62 +1156,17 @@ func (s *scanner) maybeParseFile(
path := resolveResult.PathPair.Primary
visitedKey := path
if visitedKey.Namespace == "file" {
visitedKey.Text = logger.CanonicalFileSystemPathForWindows(visitedKey.Text)
visitedKey.Text = canonicalFileSystemPathForWindows(visitedKey.Text)
}

// Only parse a given file path once
visited, ok := s.visited[visitedKey]
if ok {
// Validate that the resolved metadata for this file is the same as before
if diff := visited.resolveResult.Compare(&resolveResult); diff != nil {
prettyPath := s.res.PrettyPath(resolveResult.PathPair.Primary)
tracker := logger.MakeLineColumnTracker(importSource)
notes := make([]logger.MsgData, 0, 5+len(diff))

if visited.importSource != nil {
visitedTracker := logger.MakeLineColumnTracker(visited.importSource)
notes = append(notes, visitedTracker.MsgData(visited.importPathRange,
"The original metadata for that path comes from when it was imported here:"))
}

notes = append(notes,
logger.MsgData{Text: "The difference in metadata is displayed below:"},
logger.MsgData{},
)
for _, line := range diff {
notes = append(notes, logger.MsgData{Text: line})
}
explain := ""
if a, b := resolveResult.PluginName, visited.resolveResult.PluginName; a != "" && (a == b || b == "") {
explain += fmt.Sprintf("This is a bug in the %q plugin. ", a)
} else if a == "" && b != "" {
explain += fmt.Sprintf("This is a bug in the %q plugin. ", b)
}
explain += "Plugins provide metadata for a given path in an \"onResolve\" callback. " +
"All metadata provided for the same path must be consistent to ensure deterministic builds. " +
"Due to parallelism, one set of provided metadata will be randomly chosen for a given path, " +
"so providing inconsistent metadata for the same path can cause non-determinism."
notes = append(notes,
logger.MsgData{},
logger.MsgData{Text: explain},
)

s.log.AddMsg(logger.Msg{
Kind: logger.Error,
Data: tracker.MsgData(importPathRange,
fmt.Sprintf("Detected inconsistent metadata for the path %q when it was imported here:", prettyPath)),
Notes: notes,
PluginName: resolveResult.PluginName,
})
}
return visited.sourceIndex
}

visited = visitedFile{
sourceIndex: s.allocateSourceIndex(visitedKey, cache.SourceIndexNormal),
resolveResult: resolveResult,
importSource: importSource,
importPathRange: importPathRange,
sourceIndex: s.allocateSourceIndex(visitedKey, cache.SourceIndexNormal),
}
s.visited[visitedKey] = visited
s.remaining++
Expand Down Expand Up @@ -1374,7 +1333,7 @@ func (s *scanner) preprocessInjectedFiles() {
j := 0
for _, absPath := range s.options.InjectAbsPaths {
prettyPath := s.res.PrettyPath(logger.Path{Text: absPath, Namespace: "file"})
absPathKey := logger.CanonicalFileSystemPathForWindows(absPath)
absPathKey := canonicalFileSystemPathForWindows(absPath)

if duplicateInjectedFiles[absPathKey] {
s.log.Add(logger.Error, nil, logger.Range{}, fmt.Sprintf("Duplicate injected file %q", prettyPath))
Expand Down Expand Up @@ -1774,7 +1733,7 @@ func (s *scanner) processScannedFiles() []scannerFile {
if resolveResult.PathPair.HasSecondary() {
secondaryKey := resolveResult.PathPair.Secondary
if secondaryKey.Namespace == "file" {
secondaryKey.Text = logger.CanonicalFileSystemPathForWindows(secondaryKey.Text)
secondaryKey.Text = canonicalFileSystemPathForWindows(secondaryKey.Text)
}
if secondaryVisited, ok := s.visited[secondaryKey]; ok {
record.SourceIndex = ast.MakeIndex32(secondaryVisited.sourceIndex)
Expand Down Expand Up @@ -1834,7 +1793,7 @@ func (s *scanner) processScannedFiles() []scannerFile {
} else if !css.JSSourceIndex.IsValid() {
stubKey := otherFile.inputFile.Source.KeyPath
if stubKey.Namespace == "file" {
stubKey.Text = logger.CanonicalFileSystemPathForWindows(stubKey.Text)
stubKey.Text = canonicalFileSystemPathForWindows(stubKey.Text)
}
sourceIndex := s.allocateSourceIndex(stubKey, cache.SourceIndexJSStubForCSS)
source := logger.Source{
Expand Down Expand Up @@ -2214,12 +2173,12 @@ func (b *Bundle) Compile(log logger.Log, options config.Options, timer *helpers.
for _, sourceIndex := range allReachableFiles {
keyPath := b.files[sourceIndex].inputFile.Source.KeyPath
if keyPath.Namespace == "file" {
absPathKey := logger.CanonicalFileSystemPathForWindows(keyPath.Text)
absPathKey := canonicalFileSystemPathForWindows(keyPath.Text)
sourceAbsPaths[absPathKey] = sourceIndex
}
}
for _, outputFile := range outputFiles {
absPathKey := logger.CanonicalFileSystemPathForWindows(outputFile.AbsPath)
absPathKey := canonicalFileSystemPathForWindows(outputFile.AbsPath)
if sourceIndex, ok := sourceAbsPaths[absPathKey]; ok {
hint := ""
switch logger.API {
Expand All @@ -2246,7 +2205,7 @@ func (b *Bundle) Compile(log logger.Log, options config.Options, timer *helpers.
outputFileMap := make(map[string][]byte)
end := 0
for _, outputFile := range outputFiles {
absPathKey := logger.CanonicalFileSystemPathForWindows(outputFile.AbsPath)
absPathKey := canonicalFileSystemPathForWindows(outputFile.AbsPath)
contents, ok := outputFileMap[absPathKey]

// If this isn't a duplicate, keep the output file
Expand Down
6 changes: 0 additions & 6 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -337,12 +337,6 @@ type TSTarget struct {
TargetIsAtLeastES2022 bool
}

func (a *TSTarget) IsEquivalentTo(b *TSTarget) bool {
return (a == nil && b == nil) || (a != nil && b != nil &&
a.UnsupportedJSFeatures == b.UnsupportedJSFeatures &&
a.TargetIsAtLeastES2022 == b.TargetIsAtLeastES2022)
}

type PathPlaceholder uint8

const (
Expand Down
6 changes: 0 additions & 6 deletions internal/js_ast/js_ast.go
Original file line number Diff line number Diff line change
Expand Up @@ -1732,12 +1732,6 @@ func (mt ModuleType) IsESM() bool {
return mt >= ModuleESM_MJS && mt <= ModuleESM_PackageJSON
}

func (a ModuleType) IsEquivalentTo(b ModuleType) bool {
return (a == ModuleUnknown && b == ModuleUnknown) ||
(a.IsCommonJS() && b.IsCommonJS()) ||
(a.IsESM() && b.IsESM())
}

type ModuleTypeData struct {
Source *logger.Source
Range logger.Range
Expand Down
29 changes: 0 additions & 29 deletions internal/logger/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,24 +254,6 @@ func (a Path) ComesBeforeInSortedOrder(b Path) bool {
(a.Flags == b.Flags && a.IgnoredSuffix < b.IgnoredSuffix)))))
}

func (a Path) IsEquivalentTo(b Path) bool {
if a.Namespace == "file" {
a.Text = CanonicalFileSystemPathForWindows(a.Text)
}
if b.Namespace == "file" {
b.Text = CanonicalFileSystemPathForWindows(b.Text)
}
return a == b
}

// Identify the path by its lowercase absolute path name with Windows-specific
// slashes substituted for standard slashes. This should hopefully avoid path
// issues on Windows where multiple different paths can refer to the same
// underlying file.
func CanonicalFileSystemPathForWindows(absPath string) string {
return strings.ReplaceAll(strings.ToLower(absPath), "\\", "/")
}

var noColorResult bool
var noColorOnce sync.Once

Expand Down Expand Up @@ -1109,16 +1091,6 @@ func msgString(includeSource bool, terminalInfo TerminalInfo, kind MsgKind, data

case Note:
sb := strings.Builder{}
reset := ""

// Add special color support for rendering textual diffs
if strings.HasPrefix(data.Text, "-") {
sb.WriteString(colors.Red)
reset = colors.Reset
} else if strings.HasPrefix(data.Text, "+") {
sb.WriteString(colors.Green)
reset = colors.Reset
}

for _, line := range strings.Split(data.Text, "\n") {
// Special-case word wrapping
Expand All @@ -1141,7 +1113,6 @@ func msgString(includeSource bool, terminalInfo TerminalInfo, kind MsgKind, data
}

sb.WriteString(location)
sb.WriteString(reset)
return sb.String()
}

Expand Down
127 changes: 0 additions & 127 deletions internal/resolver/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,6 @@ type SideEffectsData struct {
type ResolveResult struct {
PathPair PathPair

// If non-empty, this was the result of an "onResolve" plugin
PluginName string

// If this was resolved by a plugin, the plugin gets to store its data here
PluginData interface{}

Expand Down Expand Up @@ -128,130 +125,6 @@ type ResolveResult struct {
UnusedImportsTS config.UnusedImportsTS
}

func prettyPrintPluginName(prefix string, key string, value string) string {
if value == "" {
return fmt.Sprintf("%s %q: null,", prefix, key)
}
return fmt.Sprintf("%s %q: %q,", prefix, key, value)
}

func prettyPrintPath(prefix string, key string, value logger.Path) string {
lines := []string{
fmt.Sprintf("%s %q: {", prefix, key),
fmt.Sprintf("%s \"text\": %q,", prefix, value.Text),
fmt.Sprintf("%s \"namespace\": %q,", prefix, value.Namespace),
}
if value.IgnoredSuffix != "" {
lines = append(lines, fmt.Sprintf("%s \"suffix\": %q,", prefix, value.IgnoredSuffix))
}
if value.IsDisabled() {
lines = append(lines, fmt.Sprintf("%s \"disabled\": true,", prefix))
}
lines = append(lines, fmt.Sprintf("%s },", prefix))
return strings.Join(lines, "\n")
}

func prettyPrintStringArray(prefix string, key string, value []string) string {
return fmt.Sprintf("%s %q: [%s],", prefix, key, helpers.StringArrayToQuotedCommaSeparatedString(value))
}

func prettyPrintTSTarget(prefix string, key string, value *config.TSTarget) string {
if value == nil {
return fmt.Sprintf("%s %q: null,", prefix, key)
}
return fmt.Sprintf("%s %q: %q,", prefix, key, value.Target)
}

func prettyPrintModuleType(prefix string, key string, value js_ast.ModuleType) string {
kind := "null"
if value.IsCommonJS() {
kind = "\"commonjs\""
} else if value.IsESM() {
kind = "\"module\""
}
return fmt.Sprintf("%s %q: %s,", prefix, key, kind)
}

func prettyPrintUnusedImports(prefix string, key string, value config.UnusedImportsTS) string {
source := "null"
switch value {
case config.UnusedImportsKeepStmtRemoveValues:
source = "{ \"importsNotUsedAsValues\": \"preserve\" }"
case config.UnusedImportsKeepValues:
source = "{ \"preserveValueImports\": true }"
}
return fmt.Sprintf("%s %q: %s,", prefix, key, source)
}

func (old *ResolveResult) Compare(new *ResolveResult) (diff []string) {
var oldDiff []string
var newDiff []string

if old.PluginName != new.PluginName {
oldDiff = append(oldDiff, prettyPrintPluginName("-", "pluginName", old.PluginName))
newDiff = append(newDiff, prettyPrintPluginName("+", "pluginName", new.PluginName))
}

if !old.PathPair.Primary.IsEquivalentTo(new.PathPair.Primary) {
oldDiff = append(oldDiff, prettyPrintPath("-", "path", old.PathPair.Primary))
newDiff = append(newDiff, prettyPrintPath("+", "path", new.PathPair.Primary))
}

if !old.PathPair.Secondary.IsEquivalentTo(new.PathPair.Secondary) {
oldDiff = append(oldDiff, prettyPrintPath("-", "secondaryPath", old.PathPair.Secondary))
newDiff = append(newDiff, prettyPrintPath("+", "secondaryPath", new.PathPair.Secondary))
}

if !helpers.StringArraysEqual(old.JSXFactory, new.JSXFactory) {
oldDiff = append(oldDiff, prettyPrintStringArray("-", "jsxFactory", old.JSXFactory))
newDiff = append(newDiff, prettyPrintStringArray("+", "jsxFactory", new.JSXFactory))
}

if !helpers.StringArraysEqual(old.JSXFragment, new.JSXFragment) {
oldDiff = append(oldDiff, prettyPrintStringArray("-", "jsxFragment", old.JSXFragment))
newDiff = append(newDiff, prettyPrintStringArray("+", "jsxFragment", new.JSXFragment))
}

if (old.PrimarySideEffectsData != nil) != (new.PrimarySideEffectsData != nil) {
oldDiff = append(oldDiff, fmt.Sprintf("- \"sideEffects\": %v,", old.PrimarySideEffectsData != nil))
newDiff = append(newDiff, fmt.Sprintf("+ \"sideEffects\": %v,", new.PrimarySideEffectsData != nil))
}

if !old.TSTarget.IsEquivalentTo(new.TSTarget) {
oldDiff = append(oldDiff, prettyPrintTSTarget("-", "tsTarget", old.TSTarget))
newDiff = append(newDiff, prettyPrintTSTarget("+", "tsTarget", new.TSTarget))
}

if !old.ModuleTypeData.Type.IsEquivalentTo(new.ModuleTypeData.Type) {
oldDiff = append(oldDiff, prettyPrintModuleType("-", "type", old.ModuleTypeData.Type))
newDiff = append(newDiff, prettyPrintModuleType("+", "type", new.ModuleTypeData.Type))
}

if old.IsExternal != new.IsExternal {
oldDiff = append(oldDiff, fmt.Sprintf("- \"external\": %v,", old.IsExternal))
newDiff = append(newDiff, fmt.Sprintf("+ \"external\": %v,", new.IsExternal))
}

if old.UseDefineForClassFieldsTS != new.UseDefineForClassFieldsTS {
oldDiff = append(oldDiff, fmt.Sprintf("- \"useDefineForClassFields\": %v,", old.UseDefineForClassFieldsTS))
newDiff = append(newDiff, fmt.Sprintf("+ \"useDefineForClassFields\": %v,", new.UseDefineForClassFieldsTS))
}

if old.UnusedImportsTS != new.UnusedImportsTS {
oldDiff = append(oldDiff, prettyPrintUnusedImports("-", "unusedImports", old.UnusedImportsTS))
newDiff = append(newDiff, prettyPrintUnusedImports("+", "unusedImports", new.UnusedImportsTS))
}

if oldDiff != nil {
diff = make([]string, 0, 2+len(oldDiff)+len(newDiff))
diff = append(diff, " {")
diff = append(diff, oldDiff...)
diff = append(diff, newDiff...)
diff = append(diff, " }")
}
return
}

type DebugMeta struct {
suggestionText string
suggestionMessage string
Expand Down
Loading

0 comments on commit a6d80fc

Please sign in to comment.