Skip to content

fix: start a namespaced tag or universal at its namespace - #359

Open
theRizwan wants to merge 1 commit into
postcss:mainfrom
theRizwan:fix/namespaced-node-source
Open

theRizwan wants to merge 1 commit into
postcss:mainfrom
theRizwan:fix/namespaced-node-source

Conversation

@theRizwan

Copy link
Copy Markdown
Contributor

The bug

A tag or universal selector with a namespace prints its namespace, but its sourceIndex and source.start point past it, at the name:

const node = parser().astSync("a ns|b").first.nodes[2];
node.toString();               // "ns|b"
node.sourceIndex;              // 5, the "b"
node.source.start.column;      // 6

So input.slice(node.sourceIndex) does not start with the node, and tools that map nodes back to the input point at the wrong place. The same happens for *|*, |a and &|a. The selector around it already starts at the namespace, and an attribute's [ns|x] starts at its [.

The fix

namespace() knows where the node starts: at the prefix token, or at the | when the namespace is empty. It now passes that token on to word()/splitWord() and universal(), which use it for the first node's sourceIndex and source.start. Where the node ends is unchanged, and so are the class and id nodes that follow it in the same word, as in ns|b.c.

Verification

Four tests in sourceIndex.mjs: a namespaced tag after a combinator, *|*, an empty namespace followed by a class, and a namespaced tag on a later line. All four fail on main.

I checked the same invariant over 50,000 random selectors: for every tag and universal node, the input sliced at sourceIndex should start with the node's own text. On main 2,306 universal nodes broke it. On this branch none do. The tag nodes that still break it are all Sass interpolations, which is #243.

npm test is green: lint, type check, and coverage gates.

The node for `ns|a` prints as `ns|a`, but its `sourceIndex` and
`source.start` pointed at the `a`, so slicing the input from `sourceIndex`
did not give the node back. The same held for `*|*`, `|a` and `&|a`. The
selector around it already started at the namespace.

The node now starts at its prefix, or at the `|` when the namespace is
empty. Where it ends is unchanged.
@MoOx

MoOx commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator

Thanks for these four. CI is green on all of them, and I reproduced each of the four defects on released 7.2.0 before reading the diffs, so none of this is in doubt. The three months #335 and #336 sat unreviewed were on me, not on you.

Two questions, and the first one is the one I actually care about.

Would you be willing to contribute the harness itself?

Your descriptions keep mentioning the thing that found these: a lossless round trip over 50,000 generated selectors in #358, and in this PR the invariant that input.slice(node.sourceIndex) must start with the node's own text, which turned up 2,306 broken universal nodes. Those are exactly the properties this project should be checking on every commit, and right now it does not: src/__tests__/ is all example-based, so every one of these defects could only be found by someone running their own sweep outside the repo.

If the generator and the invariants lived in the test suite, behind a flag or a longer-running job so they do not slow the normal run, the project would catch this class of bug itself, and a future PR in this area would come with a number anyone can reproduce. I would take that as its own PR, gladly, and it would be worth more to me than the individual fixes.

And how do you use this package?

None of your eight PRs or two issues mentions a project, a build, or a case you hit in practice, which makes me think these all come out of the sweep rather than out of something breaking on you. That is a fine way to find bugs and I am not complaining about it. But it changes how I prioritise: a defect that bites a real consumer goes in the next patch, while one that only an invariant can reach can wait for a minor. Knowing which is which, from you, saves me guessing.

On this PR specifically: it is the one of the four I want to be most careful with, because it threads a start token through namespace(), word(), splitWord() and universal(), and source positions are also the subject of the open #341 and the long-standing #243. I will come back to it with a proper review. The other three are much more contained.

@theRizwan

Copy link
Copy Markdown
Contributor Author

Yes, happy to do the harness as its own PR.

Rough plan: a seeded generator that builds selectors from fragments (namespaces, combinators, comments, escapes, Sass/Less interpolation), so any failure prints the seed and the input and you can rerun it. The checks would be the ones that found these: the lossless round trip, lossy output staying the same on a second parse, sourceIndex pointing at the node's own text, and set/print/reparse through the setters and constructors (that's where #360 and #361 came from). It'd run from its own script, something like npm run test:property, outside npm test, with an adjustable iteration count. Separate CI job if you want one.

Some of these still fail on main, for the cases in these PRs and for older ones like #243. I'd keep a list of known failures that get skipped and remove them as fixes land, instead of waiting on all of them. That ok?

On usage, you guessed right. These came from the sweep. I don't use postcss-selector-parser directly, only through Tailwind, and none of them broke anything for me. So treat all four as invariant-only.

On this one, fine by me. If it helps with #341 I can narrow the start threading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants