Repository navigation
fix(core): stop keeping both sides of an import or Dockerfile conflict - #278
Merged
Merged
Conversation
Imports: when both sides import the same module in incompatible forms (named vs default), the imports resolver declined, and the textual engine then applied the hunk as insertion_at_boundary, keeping both lines. The resolver now flags such declines as a genuine conflict, and the orchestrator reclassifies the hunk complex instead of falling through. Dockerfile: stages were paired by alias, else by image, else by a random key. A FROM changed on both sides without an alias therefore kept both stages, and a hunk with no FROM matched nothing, duplicating e.g. an ENV changed on both sides. Unaliased stages are now paired by position, and the resolver declines when the two sides have different counts of them.
The same-module/different-form check fired when the other side still had
the base import untouched (`{ x }` → `x` on one side only). Since #278
stops sending import conflicts to the textual engine, such hunks became
complex instead of resolving as before.
…ockerfile stages against base Review of #278 found hunks that main auto-resolves but the PR turned complex: - default and named imports of the same module added on each side (`import React` + `import { useState }`) are valid together; only a shared local name, or a require whose binding is not parsed, is a conflict; - when both sides modify the same base import, the textual engine cannot build a union, so the conflict flag is dropped there (restores whitespace_only and the token-level proposal). Dockerfile: mergeStage ignored base and preferred theirs, so with stages now paired by position a change made on ours only (image bump, ENV) was silently replaced. The matching base stage now settles instructions changed on one side.
…instruction lines mergeStage indexes theirs by kind:key, so a second RUN (or COPY to the same destination) replaced the first, and the base rule then emitted only the last one: `RUN a` was lost when theirs added `RUN b`. A repeated key on theirs now falls back to the textual engine, which merges it correctly as on main. So do hunks containing lines that are not instructions (a \\ continuation started before the hunk, a heredoc body), whose keys are meaningless.
With a repeated key on ours (two RUN, two ENV PATH, two EXPOSE), the same theirs instruction was matched twice: `RUN npm run build` came out twice, or a value was lost (already the case on main for ENV/EXPOSE/LABEL). Such hunks now go to the textual engine. Also: MAINTAINER is a known instruction, and a `\\` followed by spaces or `\\r` (CRLF) continues the line, so these no longer trip the non-instruction guard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two engine bugs found while checking the examples of the trivial-conflicts taxonomy article, plus a third one in the same code path.
Imports: same module in two forms kept both lines
The imports resolver correctly found the conflict (
incompatible kinds (named vs default)), but anullfrom a format-aware resolver only meant "not applicable", so the hunk went on to the textual engine. The textual engine classified itinsertion_at_boundaryand applied it, keeping both lines:useAuthwas declared twice.Fix:
ImportMergeResultandFormatResolveResultget aconflictflag, set on the resolver's semantic-conflict returns (and not on "the block is not imports" or "line cannot be parsed", which still fall through).dispatchFormatAwarereturns a newconflictstatus, andresolveHunkreclassifies the hunkcomplex(confidence and explanation from thecomplexpattern, the resolver's reason added to the trace) instead of running the textual engine.Dockerfile: stages paired by image, or by a random key
getStageKeywasalias ?? image ?? random. So:FROM node:20-alpinevsFROM node:22-alpine(no alias) did not pair → both stages kept → twoFROMlines.FROM(e.g.ENV PORT=3000vsENV PORT=4000) got a random key on each side → nothing paired → bothENV PORTlines kept. This is the most common shape and was not in the original report.Fix: stages are paired by alias when they have one, otherwise by position among unaliased stages. If the two sides do not have the same number of unaliased stages (stage added on one side, alias added on one side only), the resolver declines and the textual engine takes over.
Tests
complex, reason, a non-import block still falls through).Follow-up
The blog article
website/blog/trivial-conflicts-taxonomy.mddescribes the import case as a known gap "on the list to fix". Update it when this ships.Review fixes
A review found hunks that main auto-resolved correctly but this PR turned
complexor merged badly. Fixed in this PR, with regression tests:conflict: truenow needs a shared local name ({ useAuth }vsuseAuth,{ x as useAuth }vsuseAuth) or arequire(binding not parsed).import React+import { useState }from the same module are both kept again. A one-sided form change ({ x }→x, other side kept the base import) is not a conflict. When both sides modify the same base import, the flag is dropped: the textual engine cannot build a union there, and the token-level proposal is offered again.mergeStagenow uses the matching base stage, so an instruction changed on one side only keeps that change. Before, it was replaced by the other side's base value, which position pairing made common. The resolver now declines when ours or theirs repeats an instruction key (severalRUN,COPYto the same destination,ENV PATH): the textual engine handles those. It used to lose or duplicate one. It also declines on lines that are not instructions (heredoc bodies,\\continuations started before the hunk).MAINTAINERis a known instruction, and\\followed by spaces or\\rcontinues the line.Core suite: 1322 tests passing.
Known gaps, unchanged from main, follow-up: one-sided deletions inside a Dockerfile stage come back; COMMENT lines share one key. In imports,
oursBySourcekeeps only the last import per module, so{ a, X }+type { T }vsimport Xstill keeps both.