diff options
author | Bjørn Erik Pedersen <bjorn.erik.pedersen@gmail.com> | 2022-05-15 11:40:34 +0200 |
---|---|---|
committer | Bjørn Erik Pedersen <bjorn.erik.pedersen@gmail.com> | 2022-05-15 20:25:25 +0200 |
commit | fc9f315d86e1fe51c3d1eec3b60680113b2e3aa6 (patch) | |
tree | 69e8ffc4d84e8f02e0e6f098c27cda9dd3bcc544 | |
parent | 4b189d8fd93d3fa326b31d451d5594c917e6c714 (diff) |
Improve SASS errors
Fixes #9897
24 files changed, 306 insertions, 69 deletions
diff --git a/commands/server.go b/commands/server.go index 036fa9eaa..f61681003 100644 --- a/commands/server.go +++ b/commands/server.go @@ -482,6 +482,7 @@ func removeErrorPrefixFromLog(content string) string { var logReplacer = strings.NewReplacer( "can't", "can’t", // Chroma lexer does'nt do well with "can't" "*hugolib.pageState", "page.Page", // Page is the public interface. + "Rebuild failed:", "", ) func cleanErrorLog(content string) string { diff --git a/common/herrors/error_locator.go b/common/herrors/error_locator.go index 0f22f545f..18c21e51b 100644 --- a/common/herrors/error_locator.go +++ b/common/herrors/error_locator.go @@ -52,6 +52,16 @@ var NopLineMatcher = func(m LineMatcher) int { return 1 } +// OffsetMatcher is a line matcher that matches by offset. +var OffsetMatcher = func(m LineMatcher) int { + if m.Offset+len(m.Line) >= m.Position.Offset { + // We found the line, but return 0 to signal that we want to determine + // the column from the error. + return 0 + } + return -1 +} + // ErrorContext contains contextual information about an error. This will // typically be the lines surrounding some problem in a file. type ErrorContext struct { diff --git a/common/herrors/file_error.go b/common/herrors/file_error.go index 85007a057..e6baaf6e3 100644 --- a/common/herrors/file_error.go +++ b/common/herrors/file_error.go @@ -19,6 +19,8 @@ import ( "io" "path/filepath" + "github.com/bep/godartsass" + "github.com/bep/golibsass/libsass/libsasserrors" "github.com/gohugoio/hugo/common/paths" "github.com/gohugoio/hugo/common/text" "github.com/pelletier/go-toml/v2" @@ -132,7 +134,22 @@ func (e fileError) Position() text.Position { } func (e *fileError) Error() string { - return fmt.Sprintf("%s: %s", e.position, e.cause) + return fmt.Sprintf("%s: %s", e.position, e.causeString()) +} + +func (e *fileError) causeString() string { + if e.cause == nil { + return "" + } + switch v := e.cause.(type) { + // Avoid repeating the file info in the error message. + case godartsass.SassError: + return v.Message + case libsasserrors.Error: + return v.Message + default: + return v.Error() + } } func (e *fileError) Unwrap() error { @@ -140,9 +157,17 @@ func (e *fileError) Unwrap() error { } // NewFileError creates a new FileError that wraps err. +// It will try to extract the filename and line number from err. +func NewFileError(err error) FileError { + // Filetype is used to determine the Chroma lexer to use. + fileType, pos := extractFileTypePos(err) + return &fileError{cause: err, fileType: fileType, position: pos} +} + +// NewFileErrorFromName creates a new FileError that wraps err. // The value for name should identify the file, the best // being the full filename to the file on disk. -func NewFileError(err error, name string) FileError { +func NewFileErrorFromName(err error, name string) FileError { // Filetype is used to determine the Chroma lexer to use. fileType, pos := extractFileTypePos(err) pos.Filename = name @@ -165,6 +190,23 @@ func NewFileErrorFromPos(err error, pos text.Position) FileError { } +func NewFileErrorFromFileInErr(err error, fs afero.Fs, linematcher LineMatcherFn) FileError { + fe := NewFileError(err) + pos := fe.Position() + if pos.Filename == "" { + return fe + } + + f, realFilename, err2 := openFile(pos.Filename, fs) + if err2 != nil { + return fe + } + + pos.Filename = realFilename + defer f.Close() + return fe.UpdateContent(f, linematcher) +} + func NewFileErrorFromFileInPos(err error, pos text.Position, fs afero.Fs, linematcher LineMatcherFn) FileError { if err == nil { panic("err is nil") @@ -185,10 +227,10 @@ func NewFileErrorFromFile(err error, filename string, fs afero.Fs, linematcher L } f, realFilename, err2 := openFile(filename, fs) if err2 != nil { - return NewFileError(err, realFilename) + return NewFileErrorFromName(err, realFilename) } defer f.Close() - return NewFileError(err, realFilename).UpdateContent(f, linematcher) + return NewFileErrorFromName(err, realFilename).UpdateContent(f, linematcher) } func openFile(filename string, fs afero.Fs) (afero.File, string, error) { @@ -223,8 +265,15 @@ func Cause(err error) error { func extractFileTypePos(err error) (string, text.Position) { err = Cause(err) + var fileType string + // LibSass, DartSass + if pos := extractPosition(err); pos.LineNumber > 0 || pos.Offset > 0 { + _, fileType = paths.FileAndExtNoDelimiter(pos.Filename) + return fileType, pos + } + // Default to line 1 col 1 if we don't find any better. pos := text.Position{ Offset: -1, @@ -259,6 +308,10 @@ func extractFileTypePos(err error) (string, text.Position) { } } + if fileType == "" && pos.Filename != "" { + _, fileType = paths.FileAndExtNoDelimiter(pos.Filename) + } + return fileType, pos } @@ -322,3 +375,20 @@ func exctractLineNumberAndColumnNumber(e error) (int, int) { return -1, -1 } + +func extractPosition(e error) (pos text.Position) { + switch v := e.(type) { + case godartsass.SassError: + span := v.Span + start := span.Start + filename, _ := paths.UrlToFilename(span.Url) + pos.Filename = filename + pos.Offset = start.Offset + pos.ColumnNumber = start.Column + case libsasserrors.Error: + pos.Filename = v.File + pos.LineNumber = v.Line + pos.ColumnNumber = v.Column + } + return +} diff --git a/common/herrors/file_error_test.go b/common/herrors/file_error_test.go index 04baadf16..0b260a255 100644 --- a/common/herrors/file_error_test.go +++ b/common/herrors/file_error_test.go @@ -30,7 +30,7 @@ func TestNewFileError(t *testing.T) { c := qt.New(t) - fe := NewFileError(errors.New("bar"), "foo.html") + fe := NewFileErrorFromName(errors.New("bar"), "foo.html") c.Assert(fe.Error(), qt.Equals, `"foo.html:1:1": bar`) lines := "" @@ -70,7 +70,7 @@ func TestNewFileErrorExtractFromMessage(t *testing.T) { {errors.New(`execute of template failed: template: index.html:2:5: executing "index.html" at <partial "foo.html" .>: error calling partial: "/layouts/partials/foo.html:3:6": execute of template failed: template: partials/foo.html:3:6: executing "partials/foo.html" at <.ThisDoesNotExist>: can't evaluate field ThisDoesNotExist in type *hugolib.pageStat`), 0, 2, 5}, } { - got := NewFileError(test.in, "test.txt") + got := NewFileErrorFromName(test.in, "test.txt") errMsg := qt.Commentf("[%d][%T]", i, got) diff --git a/common/paths/url.go b/common/paths/url.go index 193b0cd0e..c538d8f2c 100644 --- a/common/paths/url.go +++ b/common/paths/url.go @@ -17,6 +17,7 @@ import ( "fmt" "net/url" "path" + "path/filepath" "strings" ) @@ -152,3 +153,29 @@ func Uglify(in string) string { // /section/name.html -> /section/name.html return path.Clean(in) } + +// UrlToFilename converts the URL s to a filename. +// If ParseRequestURI fails, the input is just converted to OS specific slashes and returned. +func UrlToFilename(s string) (string, bool) { + u, err := url.ParseRequestURI(s) + + if err != nil { + return filepath.FromSlash(s), false + } + + p := u.Path + + if p == "" { + p, _ = url.QueryUnescape(u.Opaque) + return filepath.FromSlash(p), true + } + + p = filepath.FromSlash(p) + + if u.Host != "" { + // C:\data\file.txt + p = strings.ToUpper(u.Host) + ":" + p + } + + return p, true +} @@ -11,7 +11,7 @@ require ( github.com/bep/gitmap v1.1.2 github.com/bep/goat v0.5.0 github.com/bep/godartsass v0.14.0 - github.com/bep/golibsass v1.0.0 + github.com/bep/golibsass v1.1.0 github.com/bep/gowebp v0.1.0 github.com/bep/overlayfs v0.6.0 github.com/bep/tmc v0.5.1 @@ -175,6 +175,8 @@ github.com/bep/godartsass v0.14.0 h1:pPb6XkpyDEppS+wK0veh7OXDQc4xzOJI9Qcjb743UeQ github.com/bep/godartsass v0.14.0/go.mod h1:6LvK9RftsXMxGfsA0LDV12AGc4Jylnu6NgHL+Q5/pE8= github.com/bep/golibsass v1.0.0 h1:gNguBMSDi5yZEZzVZP70YpuFQE3qogJIGUlrVILTmOw= github.com/bep/golibsass v1.0.0/go.mod h1:DL87K8Un/+pWUS75ggYv41bliGiolxzDKWJAq3eJ1MA= +github.com/bep/golibsass v1.1.0 h1:pjtXr00IJZZaOdfryNa9wARTB3Q0BmxC3/V1KNcgyTw= +github.com/bep/golibsass v1.1.0/go.mod h1:DL87K8Un/+pWUS75ggYv41bliGiolxzDKWJAq3eJ1MA= github.com/bep/gowebp v0.1.0 h1:4/iQpfnxHyXs3x/aTxMMdOpLEQQhFmF6G7EieWPTQyo= github.com/bep/gowebp v0.1.0/go.mod h1:ZhFodwdiFp8ehGJpF4LdPl6unxZm9lLFjxD3z2h2AgI= github.com/bep/overlayfs v0.6.0 h1:sgLcq/qtIzbaQNl2TldGXOkHvqeZB025sPvHOQL+DYo= diff --git a/hugolib/integrationtest_builder.go b/hugolib/integrationtest_builder.go index c3fb9fffc..0ec202f89 100644 --- a/hugolib/integrationtest_builder.go +++ b/hugolib/integrationtest_builder.go @@ -168,8 +168,9 @@ func (s *IntegrationTestBuilder) destinationExists(filename string) bool { return b } -func (s *IntegrationTestBuilder) AssertIsFileError(err error) { +func (s *IntegrationTestBuilder) AssertIsFileError(err error) herrors.FileError { s.Assert(err, qt.ErrorAs, new(herrors.FileError)) + return herrors.UnwrapFileError(err) } func (s *IntegrationTestBuilder) AssertRenderCountContent(count int) { diff --git a/hugolib/page.go b/hugolib/page.go index e6dd8fc2e..19d969532 100644 --- a/hugolib/page.go +++ b/hugolib/page.go @@ -788,7 +788,7 @@ func (p *pageState) outputFormat() (f output.Format) { func (p *pageState) parseError(err error, input []byte, offset int) error { pos := p.posFromInput(input, offset) - return herrors.NewFileError(err, p.File().Filename()).UpdatePosition(pos) + return herrors.NewFileErrorFromName(err, p.File().Filename()).UpdatePosition(pos) } func (p *pageState) pathOrTitle() string { diff --git a/hugolib/shortcode.go b/hugolib/shortcode.go index 41121700d..45d5f1ca8 100644 --- a/hugolib/shortcode.go +++ b/hugolib/shortcode.go @@ -298,7 +298,7 @@ func renderShortcode( var err error tmpl, err = s.TextTmpl().Parse(templName, templStr) if err != nil { - fe := herrors.NewFileError(err, p.File().Filename()) + fe := herrors.NewFileErrorFromName(err, p.File().Filename()) pos := fe.Position() pos.LineNumber += p.posOffset(sc.pos).LineNumber fe = fe.UpdatePosition(pos) @@ -391,7 +391,7 @@ func renderShortcode( result, err := renderShortcodeWithPage(s.Tmpl(), tmpl, data) if err != nil && sc.isInline { - fe := herrors.NewFileError(err, p.File().Filename()) + fe := herrors.NewFileErrorFromName(err, p.File().Filename()) pos := fe.Position() pos.LineNumber += p.posOffset(sc.pos).LineNumber fe = fe.UpdatePosition(pos) diff --git a/langs/i18n/translationProvider.go b/langs/i18n/translationProvider.go index fbeea71d4..d9d334567 100644 --- a/langs/i18n/translationProvider.go +++ b/langs/i18n/translationProvider.go @@ -138,6 +138,6 @@ func errWithFileContext(inerr error, r source.File) error { } defer f.Close() - return herrors.NewFileError(inerr, realFilename).UpdateContent(f, nil) + return herrors.NewFileErrorFromName(inerr, realFilename).UpdateContent(f, nil) } diff --git a/parser/metadecoders/decoder.go b/parser/metadecoders/decoder.go index 42e2ee65a..7a76b8eea 100644 --- a/parser/metadecoders/decoder.go +++ b/parser/metadecoders/decoder.go @@ -260,7 +260,7 @@ func (d Decoder) unmarshalORG(data []byte, v any) error { } func toFileError(f Format, data []byte, err error) error { - return herrors.NewFileError(err, fmt.Sprintf("_stream.%s", f)).UpdateContent(bytes.NewReader(data), nil) + return herrors.NewFileErrorFromName(err, fmt.Sprintf("_stream.%s", f)).UpdateContent(bytes.NewReader(data), nil) } // stringifyMapKeys recurses into in and changes all instances of diff --git a/resources/resource_transformers/js/build.go b/resources/resource_transformers/js/build.go index 34dae4666..23e28f675 100644 --- a/resources/resource_transformers/js/build.go +++ b/resources/resource_transformers/js/build.go @@ -165,7 +165,7 @@ func (t *buildTransformation) Transform(ctx *resources.ResourceTransformationCtx if err == nil { fe := herrors. - NewFileError(errors.New(errorMessage), path). + NewFileErrorFromName(errors.New(errorMessage), path). UpdatePosition(text.Position{Offset: -1, LineNumber: loc.Line, ColumnNumber: loc.Column}). UpdateContent(f, nil) diff --git a/resources/resource_transformers/postcss/integration_test.go b/resources/resource_transformers/postcss/integration_test.go index 69f0964d0..fdebcc52c 100644 --- a/resources/resource_transformers/postcss/integration_test.go +++ b/resources/resource_transformers/postcss/integration_test.go @@ -183,6 +183,6 @@ func TestTransformPostCSSImporSkipInlineImportsNotFound(t *testing.T) { TxtarString: files, }).Build() - s.AssertFileContent("public/css/styles.css", filepath.FromSlash(`@import "components/doesnotexist.css";`)) + s.AssertFileContent("public/css/styles.css", `@import "components/doesnotexist.css";`) } diff --git a/resources/resource_transformers/postcss/postcss.go b/resources/resource_transformers/postcss/postcss.go index 325dc1ec2..ed08f7202 100644 --- a/resources/resource_transformers/postcss/postcss.go +++ b/resources/resource_transformers/postcss/postcss.go @@ -314,7 +314,7 @@ func (imp *importResolver) importRecursive( LineNumber: offset + 1, ColumnNumber: column + 1, } - return 0, "", herrors.NewFileErrorFromFileInPos(fmt.Errorf("failed to resolve CSS @import %q", filename), pos, imp.fs, nil) + return 0, "", herrors.NewFileErrorFromFileInPos(fmt.Errorf("failed to resolve CSS @import \"%s\"", filename), pos, imp.fs, nil) } i-- @@ -421,7 +421,7 @@ func (imp *importResolver) toFileError(output string) error { } defer f.Close() - ferr := herrors.NewFileError(inErr, realFilename) + ferr := herrors.NewFileErrorFromName(inErr, realFilename) pos := ferr.Position() pos.LineNumber = file.Offset + 1 return ferr.UpdatePosition(pos).UpdateContent(f, nil) diff --git a/resources/resource_transformers/tocss/dartsass/client.go b/resources/resource_transformers/tocss/dartsass/client.go index fb794f4e8..7c3a7ecba 100644 --- a/resources/resource_transformers/tocss/dartsass/client.go +++ b/resources/resource_transformers/tocss/dartsass/client.go @@ -20,7 +20,9 @@ import ( "io" "strings" + "github.com/gohugoio/hugo/common/herrors" "github.com/gohugoio/hugo/helpers" + "github.com/gohugoio/hugo/hugofs" "github.com/gohugoio/hugo/hugolib/filesystems" "github.com/gohugoio/hugo/resources" "github.com/gohugoio/hugo/resources/resource" @@ -33,6 +35,10 @@ import ( // used as part of the cache key. const transformationName = "tocss-dart" +// See https://github.com/sass/dart-sass-embedded/issues/24 +// Note: This prefix must be all lower case. +const dartSassStdinPrefix = "hugostdin:" + func New(fs *filesystems.SourceFilesystem, rs *resources.Spec) (*Client, error) { if !Supports() { return &Client{dartSassNotAvailable: true}, nil @@ -44,7 +50,7 @@ func New(fs *filesystems.SourceFilesystem, rs *resources.Spec) (*Client, error) transpiler, err := godartsass.Start(godartsass.Options{ LogEventHandler: func(event godartsass.LogEvent) { - message := strings.ReplaceAll(event.Message, stdinPrefix, "") + message := strings.ReplaceAll(event.Message, dartSassStdinPrefix, "") switch event.Type { case godartsass.LogEventTypeDebug: // Log as Info for now, we may adjust this if it gets too chatty. @@ -94,7 +100,7 @@ func (c *Client) toCSS(args godartsass.Args, src io.Reader) (godartsass.Result, if err.Error() == "unexpected EOF" { return res, fmt.Errorf("got unexpected EOF when executing %q. The user running hugo must have read and execute permissions on this program. With execute permissions only, this error is thrown.", dartSassEmbeddedBinaryName) } - return res, err + return res, herrors.NewFileErrorFromFileInErr(err, hugofs.Os, herrors.OffsetMatcher) } return res, err diff --git a/resources/resource_transformers/tocss/dartsass/integration_test.go b/resources/resource_transformers/tocss/dartsass/integration_test.go index a1ac1d59f..c127057a5 100644 --- a/resources/resource_transformers/tocss/dartsass/integration_test.go +++ b/resources/resource_transformers/tocss/dartsass/integration_test.go @@ -14,8 +14,10 @@ package dartsass_test import ( + "strings" "testing" + qt "github.com/frankban/quicktest" "github.com/gohugoio/hugo/hugolib" "github.com/gohugoio/hugo/resources/resource_transformers/tocss/dartsass" jww "github.com/spf13/jwalterweatherman" @@ -196,3 +198,76 @@ T1: {{ $r.Content }} b.AssertLogMatches(`INFO.*Dart Sass: .*assets.*main.scss:1:0: bar`) } + +func TestTransformErrors(t *testing.T) { + if !dartsass.Supports() { + t.Skip() + } + + c := qt.New(t) + + const filesTemplate = ` +-- config.toml -- +-- assets/scss/components/_foo.scss -- +/* comment line 1 */ +$foocolor: #ccc; + +foo { + color: $foocolor; +} +-- assets/scss/main.scss -- +/* comment line 1 */ +/* comment line 2 */ +@import "components/foo"; +/* comment line 4 */ + + $maincolor: #eee; + +body { + color: $maincolor; +} + +-- layouts/index.html -- +{{ $cssOpts := dict "transpiler" "dartsass" }} +{{ $r := resources.Get "scss/main.scss" | toCSS $cssOpts | minify }} +T1: {{ $r.Content }} + + ` + + c.Run("error in main", func(c *qt.C) { + b, err := hugolib.NewIntegrationTestBuilder( + hugolib.IntegrationTestConfig{ + T: c, + TxtarString: strings.Replace(filesTemplate, "$maincolor: #eee;", "$maincolor #eee;", 1), + NeedsOsFS: true, + }).BuildE() + + b.Assert(err, qt.IsNotNil) + b.Assert(err.Error(), qt.Contains, `main.scss:8:13":`) + b.Assert(err.Error(), qt.Contains, `: expected ":".`) + fe := b.AssertIsFileError(err) + b.Assert(fe.ErrorContext(), qt.IsNotNil) + b.Assert(fe.ErrorContext().Lines, qt.DeepEquals, []string{" $maincolor #eee;", "", "body {", "\tcolor: $maincolor;", "}"}) + b.Assert(fe.ErrorContext().ChromaLexer, qt.Equals, "scss") + + }) + + c.Run("error in import", func(c *qt.C) { + b, err := hugolib.NewIntegrationTestBuilder( + hugolib.IntegrationTestConfig{ + T: c, + TxtarString: strings.Replace(filesTemplate, "$foocolor: #ccc;", "$foocolor #ccc;", 1), + NeedsOsFS: true, + }).BuildE() + + b.Assert(err, qt.IsNotNil) + b.Assert(err.Error(), qt.Contains, `_foo.scss:2:10":`) + b.Assert(err.Error(), qt.Contains, `: expected ":".`) + fe := b.AssertIsFileError(err) + b.Assert(fe.ErrorContext(), qt.IsNotNil) + b.Assert(fe.ErrorContext().Lines, qt.DeepEquals, []string{"/* comment line 1 */", "$foocolor #ccc;", "", "foo {"}) + b.Assert(fe.ErrorContext().ChromaLexer, qt.Equals, "scss") + + }) + +} diff --git a/resources/resource_transformers/tocss/dartsass/transform.go b/resources/resource_transformers/tocss/dartsass/transform.go index ba3517e26..9d17d3bcc 100644 --- a/resources/resource_transformers/tocss/dartsass/transform.go +++ b/resources/resource_transformers/tocss/dartsass/transform.go @@ -16,13 +16,12 @@ package dartsass import ( "fmt" "io" - "net/url" "path" "path/filepath" "strings" - "github.com/gohugoio/hugo/common/herrors" "github.com/gohugoio/hugo/common/hexec" + "github.com/gohugoio/hugo/common/paths" "github.com/gohugoio/hugo/htesting" "github.com/gohugoio/hugo/media" @@ -38,9 +37,6 @@ import ( ) const ( - // See https://github.com/sass/dart-sass-embedded/issues/24 - // Note: This prefix must be all lower case. - stdinPrefix = "hugostdin:" dartSassEmbeddedBinaryName = "dart-sass-embedded" ) @@ -76,7 +72,7 @@ func (t *transform) Transform(ctx *resources.ResourceTransformationCtx) error { } baseDir := path.Dir(ctx.SourcePath) - filename := stdinPrefix + filename := dartSassStdinPrefix if ctx.SourcePath != "" { filename += t.c.sfs.RealFilename(ctx.SourcePath) @@ -108,26 +104,6 @@ func (t *transform) Transform(ctx *resources.ResourceTransformationCtx) error { res, err := t.c.toCSS(args, ctx.From) if err != nil { - if sassErr, ok := err.(godartsass.SassError); ok { - start := sassErr.Span.Start - context := strings.TrimSpace(sassErr.Span.Context) - filename, _ := urlToFilename(sassErr.Span.Url) - if strings.HasPrefix(filename, stdinPrefix) { - filename = filename[len(stdinPrefix):] - } - - offsetMatcher := func(m herrors.LineMatcher) int { - if m.Offset+len(m.Line) >= start.Offset && strings.Contains(m.Line, context) { - // We found the line, but return 0 to signal that we want to determine - // the column from the error. - return 0 - } - return -1 - } - - return herrors.NewFileErrorFromFile(sassErr, filename, hugofs.Os, offsetMatcher) - - } return err } @@ -154,7 +130,7 @@ type importResolver struct { } func (t importResolver) CanonicalizeURL(url string) (string, error) { - filePath, isURL := urlToFilename(url) |