Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LazySpan doesn't follow JS SourceSpan API #1952

Closed
alan-agius4 opened this issue May 5, 2023 · 4 comments · Fixed by #2142
Closed

LazySpan doesn't follow JS SourceSpan API #1952

alan-agius4 opened this issue May 5, 2023 · 4 comments · Fixed by #2142
Assignees
Labels
bug JavaScript Issues particular to the Node.js distribution

Comments

@alan-agius4
Copy link

alan-agius4 commented May 5, 2023

The lazy span builder introduced in https://github.com/sass/dart-sass/pull/1916/files causes a bug that sometimes it's implementation is leaked when there is an exception when using the JS API.

Input

const { compileString } = require('sass');

const scss = `
@mixin dark {
  .red & {
    @content;
  }
}

@include dark {
  body {
    color: red;
  }
}
`;

try {
  compileString(scss);
} catch (e) {
  console.error(e.span);
}

Output

LazyFileSpan0 { '_lazy_file_span0$_builder': 
   Parser_spanFrom_closure0 { '$this': 
      SelectorParser0 { '_selector$_allowParent': true,
        '_selector$_allowPlaceholder': true,
        scanner: 
         SpanScanner { _sourceFile: 
            SourceFile { url: null,
              _lineStarts: [ 0, [Symbol($ti)]: [Object] ],
              _decodedChars: Uint32Array { [Iterator]  0: 46, 1: 114, 2: 101, 3: 100, 4: 32, 5: 38, 6: 32 },
              _cachedLine: null },
           sourceUrl: null,
           string: '.red & ',
           '_string_scanner$_position': 7,
           _lastMatch: null,
           _lastMatchPosition: null },
        logger: 
         DeprecationHandlingLogger0 { '_deprecation_handling$_warningCounts': 
         ....

Expected
Accessing .span returns an object matching the SourceSpan interface.

Version

npx sass --version
1.62.1 compiled with dart2js 2.19.6
@alan-agius4 alan-agius4 changed the title Lzy span builder implemention leaked in exception Lazy span builder implementation leaked in exception May 5, 2023
@nex3 nex3 added the bug label May 8, 2023
@stof
Copy link
Contributor

stof commented Oct 6, 2023

https://github.com/sass/dart-sass/blob/main/lib/src/js/source_span.dart might need to adjust the JS prototype of the LazyFileSpan

@stof
Copy link
Contributor

stof commented Oct 6, 2023

hmm, while this might be needed, I'm not sure it will solve the case of console.error, which shows the inner implementation of the class, not the public API exposed through getters...

@nex3 nex3 assigned nex3 and unassigned jathak Dec 5, 2023
@nex3
Copy link
Contributor

nex3 commented Dec 5, 2023

I'm not sure this is a bug. console.error() with a JS object is generally expected to print information about its implementation. Note that with a non-lazy span it prints:

_FileSpan {
  file: SourceFile {
    url: null,
    _lineStarts: [
      0,
      1,
      17,
      18,
      32,
      43,
      57,
      61,
      63,
      64,
      80,
      89,
      105,
      109,
      111,
      [Symbol($ti)]: [Rti]
    ],
    _decodedChars: Uint32Array(111) [
       10,  64, 101, 114, 114, 111, 114,  32,  34, 111, 104,  32,
      110, 111,  34,  59,  10,  10,  64, 109, 105, 120, 105, 110,
       32, 100,  97, 114, 107,  32, 123,  10,  32,  32,  46, 114,
      101, 100,  32,  38,  32, 123,  10,  32,  32,  32,  32,  64,
       99, 111, 110, 116, 101, 110, 116,  59,  10,  32,  32, 125,
       10, 125,  10,  10,  64, 105, 110,  99, 108, 117, 100, 101,
       32, 100,  97, 114, 107,  32, 123,  10,  32,  32,  98, 111,
      100, 121,  32, 123,  10,  32,  32,  32,  32,  99, 111, 108,
      111, 114,  58,  32,
      ... 11 more items
    ],
    _cachedLine: 1
  },
  '_file$_start': 1,
  _end: 15
}

which is certainly still full of implementation details. We could customize this by defining a util.inspect.custom method for Dart's span types, but I'm not sure that's really worth doing, any more than it's worth defining one for any other arbitrary object type exported by any other arbitrary library. You could as easily write console.error(e.span.toString()) and get a more human-friendly output.

@nex3 nex3 closed this as not planned Won't fix, can't repro, duplicate, stale Dec 5, 2023
@nex3
Copy link
Contributor

nex3 commented Dec 5, 2023

Actually, diving into angular/angular-cli#25134, it looks like the original issue wasn't so much console.error() as that the documented JS getters don't work for LazySpan. That we can fix.

@nex3 nex3 reopened this Dec 5, 2023
@nex3 nex3 added the JavaScript Issues particular to the Node.js distribution label Dec 5, 2023
@nex3 nex3 changed the title Lazy span builder implementation leaked in exception LazySpan doesn't follow JS SourceSpan API Dec 5, 2023
nex3 added a commit that referenced this issue Dec 5, 2023
@nex3 nex3 closed this as completed in #2142 Dec 5, 2023
nex3 added a commit that referenced this issue Dec 5, 2023
lmeysel pushed a commit to lmeysel/dart-sass that referenced this issue Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug JavaScript Issues particular to the Node.js distribution
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants