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

Computed column numbers are not consistent with editors for surrogate pairs #74

Open
stof opened this issue Mar 12, 2021 · 3 comments
Open
Labels
contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue)

Comments

@stof
Copy link

stof commented Mar 12, 2021

I tested that with the error reporting of dart-sass, which relies on that package (using SourceFile.fromString).

Giving it this invalid input, it reports an error on the ;:

a {
  b: "👭a"(;
}
Error: expected ")".
  ╷
2 │   b: "👭a"(;
  │            ^
  ╵
  test.scss 2:12  root stylesheet

The error is reported as being at column 12. However, all editors I tried (VS Code, Intellij IDEs, gedit) are reporting this ; as being at column 11. They seem to compute columns based on unicode codepoints (or maybe on glyphs), not based on UTF-16 code units.

@stof stof changed the title Computed column numbers are not consistent editors for surrogate pairs Computed column numbers are not consistent with editors for surrogate pairs Mar 12, 2021
@natebosch
Copy link
Member

Yeah this package was written before characters existed and it uses length and codeUnitAt which I think don't handle surrogate pairs, just UTF-16 code units.

@lrhn - would you expect that migrating away from the native String APIs onto characters will solve this?

@lrhn
Copy link
Member

lrhn commented Mar 15, 2021

I think you will likely need both.

The source-span represents a slice of source. Source can be represented as UTF-8 code units, UTF-16 code units or a string. It sounds like it uses UTF-16 code units as the default representation. For that, the length should be in code units. That's the correct length in the source. That's what tells you which characters you need, in an easily accessible way.
Making the source use Characters is an unnecessary complication for that.

For printing, and showing offsets to users, you are more likely to want user-perceived characters. The offset into the line should probably be counted in grapheme clusters.
So, a source span is just a range of code points. The position in the file, in user understandable line/position numbers, the position should be computed from using characters/glyphs.

For printing the ^, you don't need the length of the span at all, you need the size of the printed glyphs as rendered. That may be one character unit-size per grapheme cluster, but that assumes a fixed-width font, and even if the font is fixed-width, the emojis aren't necessarily the same size. The only really safe way to align the ^ on a new line is to print the same preceding characters again in invisible ink (but since ANSI conceal is not well supported, that's likely to just be black-on-black or white-on-white). If you have that kind of formatting available, maybe just use ANSI underline instead.

@stof
Copy link
Author

stof commented Mar 15, 2021

I'm not talking about the position of the ^ here (even though it is indeed also off, but fixing it in span.highlight would probably be complex due to the size of printed glyphs).
I'm talking about the column reported in the Sass stack trace, using span.start.column. That's the 12 in test.scss 2:12, which should be 11 according to what all editors I tried are using as position for that char.

@natebosch natebosch added the contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) label Jun 15, 2021
natebosch added a commit that referenced this issue May 25, 2022
Towards #74

Use `Characters` to operate based on grapheme clusters instead of code
units.
nex3 pushed a commit to nex3/source_span that referenced this issue Jul 2, 2024
…dart-lang#74)

Bumps the github-actions group with 1 update: [actions/checkout](https://github.com/actions/checkout).

Updates `actions/checkout` from 4.1.4 to 4.1.5
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a href="https://github.com/actions/checkout/releases">actions/checkout's releases</a>.</em></p>
<blockquote>
<h2>v4.1.5</h2>
<h2>What's Changed</h2>
<ul>
<li>Update NPM dependencies by <a href="https://github.com/cory-miller"><code>@�cory-miller</code></a> in <a href="https://redirect.github.com/actions/checkout/pull/1703">actions/checkout#1703</a></li>
<li>Bump github/codeql-action from 2 to 3 by <a href="https://github.com/dependabot"><code>@�dependabot</code></a> in <a href="https://redirect.github.com/actions/checkout/pull/1694">actions/checkout#1694</a></li>
<li>Bump actions/setup-node from 1 to 4 by <a href="https://github.com/dependabot"><code>@�dependabot</code></a> in <a href="https://redirect.github.com/actions/checkout/pull/1696">actions/checkout#1696</a></li>
<li>Bump actions/upload-artifact from 2 to 4 by <a href="https://github.com/dependabot"><code>@�dependabot</code></a> in <a href="https://redirect.github.com/actions/checkout/pull/1695">actions/checkout#1695</a></li>
<li>README: Suggest <code>user.email</code> to be <code>41898282+github-actions[bot]@users.noreply.github.com</code> by <a href="https://github.com/cory-miller"><code>@�cory-miller</code></a> in <a href="https://redirect.github.com/actions/checkout/pull/1707">actions/checkout#1707</a></li>
</ul>
<p><strong>Full Changelog</strong>: <a href="https://github.com/actions/checkout/compare/v4.1.4...v4.1.5">https://github.com/actions/checkout/compare/v4.1.4...v4.1.5</a></p>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/actions/checkout/commit/44c2b7a8a4ea60a981eaca3cf939b5f4305c123b"><code>44c2b7a</code></a> README: Suggest <code>user.email</code> to be `41898282+github-actions[bot]<a href="https://github.com/users"><code>@�users</code></a>.norepl...</li>
<li><a href="https://github.com/actions/checkout/commit/8459bc0c7e3759cdf591f513d9f141a95fef0a8f"><code>8459bc0</code></a> Bump actions/upload-artifact from 2 to 4 (<a href="https://redirect.github.com/actions/checkout/issues/1695">#1695</a>)</li>
<li><a href="https://github.com/actions/checkout/commit/3f603f6d5e9f40714f97b2f017aa0df2a443192a"><code>3f603f6</code></a> Bump actions/setup-node from 1 to 4 (<a href="https://redirect.github.com/actions/checkout/issues/1696">#1696</a>)</li>
<li><a href="https://github.com/actions/checkout/commit/fd084cde189b7b76ec305d52e27be545a0172823"><code>fd084cd</code></a> Bump github/codeql-action from 2 to 3 (<a href="https://redirect.github.com/actions/checkout/issues/1694">#1694</a>)</li>
<li><a href="https://github.com/actions/checkout/commit/9c1e94e0ad997d618b6113a2171b055037589028"><code>9c1e94e</code></a> Update NPM dependencies (<a href="https://redirect.github.com/actions/checkout/issues/1703">#1703</a>)</li>
<li>See full diff in <a href="https://github.com/actions/checkout/compare/0ad4b8fadaa221de15dcec353f45205ec38ea70b...44c2b7a8a4ea60a981eaca3cf939b5f4305c123b">compare view</a></li>
</ul>
</details>
<br />

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=actions/checkout&package-manager=github_actions&previous-version=4.1.4&new-version=4.1.5)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency
- `@dependabot ignore <dependency name> major version` will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself)
- `@dependabot ignore <dependency name> minor version` will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself)
- `@dependabot ignore <dependency name>` will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself)
- `@dependabot unignore <dependency name>` will remove all of the ignore conditions of the specified dependency
- `@dependabot unignore <dependency name> <ignore condition>` will remove the ignore condition of the specified dependency and ignore conditions

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue)
Projects
None yet
Development

No branches or pull requests

3 participants