ReviewOS

stacks/stx

codemod: the confirm rule matches a locally-declared confirm(), so ConfirmDialog.stx is told to adopt a confirm dialog

#1898
Closed glennmichael123 opened this 14 days ago · 1 comment
14 days ago

The confirm rule matches the identifier confirm without checking it resolves to the browser global, so a component that declares its own confirm function is reported as hand-rolled code — including the component whose entire purpose is to be the replacement.

From stacksjs/stacks, resources/components/Dashboard/UI/ConfirmDialog.stx:

function confirm(): void {
  if (!busy())
    emit('confirm')
}
ConfirmDialog.stx:18  [confirm]  function confirm(): void {
ConfirmDialog.stx:??  [confirm]  <button @click="confirm()">

Both hits are the component's own function — the declaration and a template call to it. Neither has anything to do with window.confirm.

Reduced:

<script client>
function confirm(): void {
  emit('confirm')
}
</script>
<button @click="confirm()">Yes</button>

→ two findings, both wrong.

Severity is low, and deliberately so

--fix does not corrupt this. It declines:

0 file(s) written.
  stxConfirm() returns a Promise and native confirm() is synchronous, so this needs `await`
  inside an async function. Rewriting it here would make the condition always true.

That async/sync guard happens to catch this case as a side effect, so the damage is confined to the report. I checked specifically because a rewritable rule firing on a declaration would otherwise rename the function and break every caller — worth stating that the guard is what prevents it, since a future relaxation of that guard would turn this into a miscompile.

Suggested narrowing

Two cheap checks, either of which kills both hits:

  • skip when the identifier is in a declaration position (function confirm, const confirm =, a method name) rather than a call in value position
  • skip when the file declares confirm in scope, since the local shadows the global and the call cannot be the browser dialog

The second generalises better — the same shadowing argument applies to any rule keyed on a bare global name.

Raised because #1843's close-out reported the corpus as clean ("no false positive I can still find") after narrowing two detectors against 191 files. This is a third, and it is the kind that erodes trust fastest: the tool tells the team that wrote the framework's confirm dialog that they should be using a confirm dialog.

14 days ago

Fixed in 2bda950b32.

The rule now bails on any file that declares its own confirm — a function declaration, a const/let bound to one, or a destructured import. ConfirmDialog.stx reports nothing.

Bailing on the file rather than resolving scopes is deliberate: the shadowing declaration and the calls are routinely in different blocks of the same .stx file — a <script client> function and a @click in the markup — so anything narrower has to model scope across the block boundary to get the common case right. The cost is a missed rewrite in a file that both declares a confirm and separately calls the global one, which is not a thing anyone writes.

Agreed on your severity read, and it is the reason this was worth fixing anyway: --fix declined both hits, so nothing was ever corrupted. But a tool that tells you to replace a thing with itself is a tool people stop reading, and being read is the entire value of this codemod — #1843's finding was that the primitives ship and get used zero times.

4 tests, including that emit('confirm') does not count as a declaration (it is a string, not a binding) and that the rule still fires normally in a file with no local confirm.

Sign in to comment on this issue.