fix(extraction): reach a Swift URL built by a constructor - #1976
fix(extraction): reach a Swift URL built by a constructor#1976CaptainMittens wants to merge 2 commits into
Conversation
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
90dd4de to
458f9fc
Compare
|
Reviewed. The extraction change is good and I have nothing to ask of it.
Pulling the Three things about the stack, so you can sequence it without guessing. 1. #1896 should go first, and it can go on its own. It is the base layer, it is self-contained, and — unlike this one — it touches no vendored file, so it has no manifest obligation. Once it merges, this PR narrows to its own commit and gets much easier to read. 2. This PR needs a 3. #1977 is closed — #1986 supersedes it, and that changes one of your assumptions here. #1986 is by you and strictly larger: it fixes The consequence for this branch: this PR's description says the carried grammar commit is byte-identical to the standalone one and will therefore merge without conflict. That was true of #1977 but is not true of #1986 — the superset has a hunk yours does not. So if #1986 lands first, expect this branch to need a rebase rather than to shrink on its own. Worth knowing before it surprises you. One unrelated heads-up: Thanks for splitting the grammar fix out into its own PR rather than leaving it buried in this one, and for laying the stack out in the description — the commit-to-PR table made this quick to follow. |
DeusData
added
bug
The Swift scanner keeps a 64-bit mask of the symbols that suppress a
match -- the rule that stops `try!` emitting its `!` as a token of its
own. It tests one bit per candidate:
uint64_t suppressing_symbols = OP_SYMBOL_SUPPRESSOR[full_match];
for (uint64_t suppressor = 0; suppressor < TOKEN_COUNT; suppressor++) {
if (!(suppressing_symbols & 1 << suppressor)) {
The mask is uint64_t but the literal `1` is an int, so the shift is an
int shift. TOKEN_COUNT is larger than 32, so once suppressor reaches 31
the shift runs past the width of the type. That is undefined behavior,
and every bit above 31 is tested against a value the standard does not
define.
Nothing caught it because nothing in the tree reached the suppressor
path. Any Swift force-unwrap does: `cached!` is enough.
UBSan reports it as:
scanner.c:514:47: runtime error: left shift of 1 by 31 places
cannot be represented in type 'int'
`1ULL` makes the literal as wide as the mask it is tested against.
The new test in tests/test_extraction.c cannot go red on its own. The
normal test build prints the UBSan message and carries on, which is why
this survived. The Windows CLANGARM64 leg runs UBSan in trap mode, and
there the same shift is an illegal-instruction crash -- so the test
exists to make sure that leg keeps parsing a force-unwrap at all.
scripts/vendored-checksums.txt records the new hash for the one changed
file, as scripts/security-vendored.sh --update writes it. Layer 8 of the
security gate compares vendored content against that manifest, so the
edit and its recorded hash belong in the same commit.
Found while adding Swift URL extraction in DeusData#1892 / DeusData#1976, and split out
of that PR so the vendored change can be reviewed on its own.
Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
The Swift scanner keeps a 64-bit mask of the symbols that suppress a
match -- the rule that stops `try!` emitting its `!` as a token of its
own. It tests one bit per candidate:
uint64_t suppressing_symbols = OP_SYMBOL_SUPPRESSOR[full_match];
for (uint64_t suppressor = 0; suppressor < TOKEN_COUNT; suppressor++) {
if (!(suppressing_symbols & 1 << suppressor)) {
The mask is uint64_t but the literal `1` is an int, so the shift is an
int shift. TOKEN_COUNT is larger than 32, so once suppressor reaches 31
the shift runs past the width of the type. That is undefined behavior,
and every bit above 31 is tested against a value the standard does not
define.
Nothing caught it because nothing in the tree reached the suppressor
path. Any Swift force-unwrap does: `cached!` is enough.
UBSan reports it as:
scanner.c:514:47: runtime error: left shift of 1 by 31 places
cannot be represented in type 'int'
`1ULL` makes the literal as wide as the mask it is tested against.
The new test in tests/test_extraction.c cannot go red on its own. The
normal test build prints the UBSan message and carries on, which is why
this survived. The Windows CLANGARM64 leg runs UBSan in trap mode, and
there the same shift is an illegal-instruction crash -- so the test
exists to make sure that leg keeps parsing a force-unwrap at all.
scripts/vendored-checksums.txt records the new hash for the one changed
file, as scripts/security-vendored.sh --update writes it. Layer 8 of the
security gate compares vendored content against that manifest, so the
edit and its recorded hash belong in the same commit.
Found while adding Swift URL extraction in DeusData#1892 / DeusData#1976, and split out
of that PR so the vendored change can be reviewed on its own.
Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
458f9fc to
82e1de8
Compare
|
Manifest row added, branch rebuilt on current The stale assumption, and what I did with itYou were right that "byte-identical, so it merges without conflict" was true of #1977 and false of #1986. Rather than delete the claim, I made it true again: this branch now carries #1986's three commits in full — I checked that rather than assuming it. Diffing That also answers your manifest point: the row arrives with #1986's commit rather than as a separate edit here, so the two branches cannot drift into two different rows for one patch. Current stack
Merge order as you set it: #1896, then #1986, then this one narrows to a single commit. Verified on the rebuilt branch with On the couplingTaken, and it was fair. Five open PRs across two stacks with shared files is how a stale assumption survived into a description in the first place — I could not hold all of it, so I stopped re-reading the parts I thought I already knew. I am opening nothing further on these stacks until #1896 and #1986 are in. |
|
Status update: the thing this was waiting on has landed, and that is also why it now conflicts. #1986 merged earlier today as A rebase onto There is also the chain to #1896 ("reach Swift call arguments through My earlier review stands: reviewed and cleared, blocked only on sequencing. Nothing about the change itself needs to move. Sorry for the wait — the queue is finally moving properly today, and your Swift work is a good chunk of what has landed: #1986 merged, and #2008, #2009, #1875, #1877 besides. |
handle_calls reads a call's arguments through one tree-sitter field lookup, ts_node_child_by_field_name(node, "arguments"). The vendored Swift grammar declares no "arguments" field at all -- its own ts_field_names[] table has zero occurrences, against one in Go and two in TypeScript. Swift models a call as a target expression plus a call_suffix, and the arguments hang off the suffix as value_arguments. So args was null for every Swift call ever parsed, first_string_arg was never populated, and no Swift HTTP call could raise an HTTP_CALLS edge or a Route node. Alamofire, Moya and URLSession have been in the service-pattern table the whole time and match the callee text correctly; the URL simply never arrived. Three changes, all in extract_calls.c: - swift_call_args() reaches the argument list through call_suffix, in the same shape as the existing objectscript_call_args() fallback and used from the same place. A trailing closure has no value_arguments, so it returns a null node and that call behaves as before. - extract_url_or_topic_arg() unwraps Swift's per-argument value_argument node, stepping past a leading value_argument_label. Without it, dataTask(with: "/api/v1/widgets") yields the label "with" rather than the path. PHP and C# already had the same unwrap for their own "argument" node. - is_string_like() gains line_string_literal, which is what Swift calls an ordinary "..." literal. The list already held raw_string_literal, so only Swift's common case was missing. This is the layer underneath DeusData#1892 rather than the whole of it. A literal URL argument now arrives; a literal nested inside a constructor, as in URLSession.shared.data(from: URL(string: "...")!), still does not, because extract_positional_url reads a literal, a template string, a concatenation or a named constant and that shape is none of them. Reproduce-first: all three tests fail without the fix, two on a null first_string_arg and one on HTTP_CALLS being 0. Fixes DeusData#1892 Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
Swift has no URL literal, so almost no real code passes a bare string to a request. It writes URL(string: "https://…")! instead, and the literal then sits two levels below the argument list: past the trailing "!", which the grammar models as a postfix_expression, and inside the constructor's own value_arguments. extract_url_or_topic_arg saw only the outer node and gave up, so the URL never reached the service-pattern table and no Route node formed. That is the shape issue DeusData#1892 reported from a real project — reaching a bare string argument was only the layer underneath it. swift_unwrap_url_constructor() steps past both wrappers. It unwraps only URL, URLComponents and URLRequest, so any other constructor keeps its own meaning and the outer call does not borrow the inner call's string. A non-literal argument such as URL(string: base + path) falls through to the ordinary handling unchanged. The value_argument unwrap added for the bare-string case is now swift_argument_value(), because the nested argument list needs the same step and the code was identical. Refs DeusData#1892 Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
82e1de8 to
7b528b3
Compare
|
Rebased — #1896 first, then this branch on top of it. Both are green on my machine and CI is re-running. This PR is now the two commits you described: #1896's One detail worth recording, because it changes what a plain rebase does here. Of the three #1986 commits this branch carried, only the try-bang one matched its merged counterpart by patch-id. The int-shift commit and the I also rewrote the description. It still described the five-commit stack and explained why the branch carried the vendored diff, and neither is true now. Local run after the rebase, sanitized build (ASan + UBSan): extraction 335 passed, pipeline 266 passed, no failures. That run includes |
2 participants
Swift has no URL literal, so real code almost never passes a bare string to a request. It writes
URL(string: "https://…")!, and the literal then sits two levels below the argument list — past the trailing!, which the grammar models as apostfix_expression, and inside the constructor's ownvalue_arguments.extract_url_or_topic_argsaw only the outer node and gave up, so the URL never reached the service-pattern table and no Route node formed. That is the shape #1892 reported from a real project.This PR stacks on #1896
Rebased on 2026-09-02, after #1986 merged as
620613ed. The branch is now two commits:call_suffixGitHub cannot base a cross-fork pull request on a branch in the fork, so #1896's commit still shows here. It disappears on its own when #1896 merges.
#1896 is the layer underneath: Swift models a call as a target plus a
call_suffix, soargswas null for every Swift call and no URL ever arrived. Reaching a bare string argument was only half the problem — this PR reaches the constructor that wraps it.The fix
swift_unwrap_url_constructor()steps past both wrappers. It unwraps onlyURL,URLComponentsandURLRequest, so any other constructor keeps its own meaning and the outer call does not borrow the inner call's string. A non-literal argument such asURL(string: base + path)falls through to the ordinary handling unchanged.The
value_argumentunwrap added for the bare-string case becameswift_argument_value(), because the nested argument list needs the same step and the code was identical.The vendored grammar fix now comes from main
An earlier version of this branch carried #1986's three commits — the two Swift scanner shift corrections and the
MANIFEST.mdrow — because this PR's test is the first thing in the tree to reach the Swift scanner's suppressor path, the rule that stopstry!emitting its!as its own token, which a force-unwrappedURL(...)!hits.#1986 has merged, so the branch no longer carries any of that. This PR touches no vendored file: the diff is
internal/cbm/extract_calls.c,tests/test_extraction.candtests/test_pipeline.conly.Tests
Three in
tests/test_extraction.c, one of them a negative control:swift_nested_url_constructor_issue1892URL(string: "…")!— the force-unwrapped shape from the reportswift_nested_url_no_bang_issue1892URLRequest(url: "…")— no!, so nopostfix_expressionwrapperswift_non_url_constructor_untouched_issue1892Formatter(pattern: "%s-%d")must yield no string, so the unwrap cannot over-reachPlus a pipeline test in
tests/test_pipeline.cthat the Route node now forms.Local run after the rebase, sanitized build (ASan + UBSan): extraction 335 passed, pipeline 266 passed, no failures.
Refs #1892