refactor(linter): remove AstKind for SimpleAssignmentTarget

[taearls]

Part of #11490

Progress Checklist:

• Removed SimpleAssignmentTarget from the ENUMS_WHITE_LIST
• regenerated the AST definitions
• addressed the compile errors in oxc_semantic, oxc_formatter, oxc_ast, and oxc_mangler
• all unit tests passing
• all integration tests passing
• just ready command runs successfully
• fix conformance test failures (left comment for review)
• self-review PR to see if I can simplify / minimize my changes

Snapshot updates:

• After removing SimpleAssignmentTarget, the node ids in several snapshots were updated, which is expected since the generated node ids were changed.
• the prettier_conformance test snapshot for TS is slightly different (left a comment for feedback below)

[codspeed-hq]

CodSpeed Instrumentation Performance Report

Merging #12401 will not alter performance

_{Comparing taearls:07_19_refactor_remove_ast_kind_from_simple_assignment_target (4cb9a80) with main (3cdac4c)}

Summary

✅ 34 untouched benchmarks

[overlookmotel]

I thought I saw a minute ago a diff in snapshots, a lint rule erroring on the !! in something like if (x || !!(y || z)). But now it's gone!

That looked like a pattern that the rule maybe should be catching - the !!(...) is unnecessary - so I thought you might have surfaced an unrelated bug. Can you remember what it was exactly? We might want to fix that separately.

[taearls]

I thought I saw a minute ago a diff in snapshots, a lint rule erroring on the !! in something like if (x || !!(y || z)). But now it's gone!

That looked like a pattern that the rule maybe should be catching - the !!(...) is unnecessary - so I thought you might have surfaced an unrelated bug. Can you remember what it was exactly? We might want to fix that separately.

@overlookmotel I think you're talking about this.

The snapshot update was reverted once I had applied the change that you suggested the other day, which was to remove the self.leave_node(kind) from visit_simple_assignment_target in builder.rs.

My understanding was that the bug from my incomplete changes had caused the reporting in the snapshot to change, but it's very possible that I could be missing something here.

[overlookmotel]

@overlookmotel I think you're talking about this.

Ah ha! The diff I saw was those same lines but all in green. But, as you've explained, that was reverting a previous change. So we're all good. Sorry, my question was a complete red herring.

[taearls]

@overlookmotel I think you're talking about this.

Ah ha! The diff I saw was those same lines but all in green. But, as you've explained, that was reverting a previous change. So we're all good. Sorry, my question was a complete red herring.

All good, thanks for looking out! My goal has been to get the tests passing without changing any of the snapshots. Ideally this refactor should not influence the functionality for end users.

To that end, once I get everything working, I want to also take a few cycles to look for opportunities to simplify / minimize my changes. I'll likely leave comments on my PR on anything I'm not sure about.

[overlookmotel]

To that end, once I get everything working, I want to also take a few cycles to look for opportunities to simplify / minimize my changes. I'll likely leave comments on my PR on anything I'm not sure about.

Thank you for your very thorough approach. It's really appreciated.

FYI: What we've noticed while making other changes to AstKind is that the test coverage in linter is often not as good as it could be. Unfortunately, it's quite easy to introduce subtle bugs, and all tests may still pass. If you have time, it's ideal to go through the changes to linter and try to deduce the logic of the code, and make sure it's not changing, rather than relying solely on the tests.

This is our bad - other parts of Oxc have very thorough test coverage, which we can lean on when making changes, but the linter is less comprehensively tested. We might want to improve that (oxc-project/backlog#170), but right now this is where we are.

However, figuring out the logic of each lint rule and trying to imagine all possible edge cases is both difficult and onerous, and tricky if you're not familiar with what the lint rules do. If you don't have time to delve that deep, please feel free to leave this until review.

Another side note: If you come across any unrelated bugs you want to fix, or edge cases that you want to add tests for, it'd be ideal to do that in separate PRs. This PR has quite a large diff, so it'd be preferable for ease of review if it's 100% refactor.

Hope this is useful background.

[camc314]

note to self, we need to fix all of the ecosystem CI failures before merging

https://github.com/oxc-project/oxlint-ecosystem-ci/actions/runs/16538642569

(excluding oven-sh/bun as it's a diff issue)

[camc314]

Another false positive:

export class EdgelessColorPickerButton extends WithDisposable(LitElement) {
  readonly #select = (e: ColorEvent) => {
    e.stopPropagation();
    this.#pick(e.detail);
  };

  override render() {
    return html`
      <editor-menu-button
        .contentPadding=${this.tabContentPadding}
        .button=${html`
          <editor-icon-button
            aria-label=${this.label}
            .tooltip=${this.tooltip || this.label}
          >
            ${when(
              this.isText,
              () => html`
                <edgeless-text-color-icon
                  .color=${this.colorWithoutAlpha}
                ></edgeless-text-color-icon>
              `,
              () => html`
                <edgeless-color-button
                  .color=${this.colorWithoutAlpha}
                  .hollowCircle=${this.hollowCircle}
                ></edgeless-color-button>
              `
            )}
          </editor-icon-button>
        `}
      >
        ${choose(this.tabType, [
          [
            'normal',
            () => html`
              <div data-orientation="vertical">
                <slot name="other"></slot>
                <slot name="separator"></slot>
                <edgeless-color-panel
                  role="listbox"
                  class=${ifDefined(this.colorPanelClass)}
                  .value=${this.color}
                  .theme=${this.theme}
                  .palettes=${this.palettes}
                  .hollowCircle=${this.hollowCircle}
                  .hasTransparent=${false}
                  @select=${this.#select}
                >
                  ${when(
                    this.enableCustomColor,
                    () => html`
                      <edgeless-color-custom-button
                        slot="custom"
                        style=${styleMap(this.customButtonStyle)}
                        ?active=${this.isCustomColor}
                        @click=${this.switchToCustomTab}
                      ></edgeless-color-custom-button>
                    `
                  )}
                </edgeless-color-panel>
              </div>
            `,
          ],
          [
            'custom',
            () => {
              const packed = packColorsWith(
                this.theme,
                this.color,
                this.originalColor
              );
              const type = packed.type === 'palette' ? 'normal' : packed.type;
              const modes = packed.colors.map(
                preprocessColor(window.getComputedStyle(this))
              );

              return html`
                <edgeless-color-picker
                  class="custom"
                  .pick=${this.pick}
                  .colors=${{ type, modes }}
                ></edgeless-color-picker>
              `;
            },
          ],
        ])}
      </editor-menu-button>
    `;
  }

}

  ⚠ eslint(no-unused-private-class-members): 'select' is defined but never used.
   ╭─[no_unused_private_class_members.tsx:2:12]
 1 │ export class EdgelessColorPickerButton extends WithDisposable(LitElement) {
 2 │   readonly #select = (e: ColorEvent) => {
   ·            ───────
 3 │     e.stopPropagation();
   ╰────

[taearls]

Another false positive:

export class EdgelessColorPickerButton extends WithDisposable(LitElement) {
  readonly #select = (e: ColorEvent) => {
    e.stopPropagation();
    this.#pick(e.detail);
  };

  override render() {
    return html`
      <editor-menu-button
        .contentPadding=${this.tabContentPadding}
        .button=${html`
          <editor-icon-button
            aria-label=${this.label}
            .tooltip=${this.tooltip || this.label}
          >
            ${when(
              this.isText,
              () => html`
                <edgeless-text-color-icon
                  .color=${this.colorWithoutAlpha}
                ></edgeless-text-color-icon>
              `,
              () => html`
                <edgeless-color-button
                  .color=${this.colorWithoutAlpha}
                  .hollowCircle=${this.hollowCircle}
                ></edgeless-color-button>
              `
            )}
          </editor-icon-button>
        `}
      >
        ${choose(this.tabType, [
          [
            'normal',
            () => html`
              <div data-orientation="vertical">
                <slot name="other"></slot>
                <slot name="separator"></slot>
                <edgeless-color-panel
                  role="listbox"
                  class=${ifDefined(this.colorPanelClass)}
                  .value=${this.color}
                  .theme=${this.theme}
                  .palettes=${this.palettes}
                  .hollowCircle=${this.hollowCircle}
                  .hasTransparent=${false}
                  @select=${this.#select}
                >
                  ${when(
                    this.enableCustomColor,
                    () => html`
                      <edgeless-color-custom-button
                        slot="custom"
                        style=${styleMap(this.customButtonStyle)}
                        ?active=${this.isCustomColor}
                        @click=${this.switchToCustomTab}
                      ></edgeless-color-custom-button>
                    `
                  )}
                </edgeless-color-panel>
              </div>
            `,
          ],
          [
            'custom',
            () => {
              const packed = packColorsWith(
                this.theme,
                this.color,
                this.originalColor
              );
              const type = packed.type === 'palette' ? 'normal' : packed.type;
              const modes = packed.colors.map(
                preprocessColor(window.getComputedStyle(this))
              );

              return html`
                <edgeless-color-picker
                  class="custom"
                  .pick=${this.pick}
                  .colors=${{ type, modes }}
                ></edgeless-color-picker>
              `;
            },
          ],
        ])}
      </editor-menu-button>
    `;
  }

}

  ⚠ eslint(no-unused-private-class-members): 'select' is defined but never used.
   ╭─[no_unused_private_class_members.tsx:2:12]
 1 │ export class EdgelessColorPickerButton extends WithDisposable(LitElement) {
 2 │   readonly #select = (e: ColorEvent) => {
   ·            ───────
 3 │     e.stopPropagation();
   ╰────

I'm curious - were there additional test cases in the main branch that also needed to be covered here? I'm not sure I follow the changes that were made.

[camc314]

i should hopefully have fixed them all.

we have an ecosystem CI, which i just ran in this branch to flag these additional errors. that's the link to the workflow above