Update swc

[ef4]

This merges latest SWC into our fork and resolves all the obvious issues.

There is one test failure relating to the hygiene system.

@chancancode, you might want to rebase #72 on this, as your work may already address the test failure.

[chancancode]

I tried rebasing but it doesn't fix the issue and may have introduced new ones (didn't dig too deep to check if it's a rebase error or what).

Looking at the swc commits, it maybe this one that broke things: embroider-build/swc@a65be14#diff-478d9b80f00e243b98f4aff45f6236d77e986d7098fdbebd2ddce73208af5754

[chancancode]

I tried this test case, #71 does seem to be fixed on this branch:

testcase! {
  handles_typescript,
  r#"function foo(this: void, message: string) {
       return function bar(this: void, message: string) {
         console.log(message);
       };
     }
     function makeTemplate() {
       return <template>hello</template>;
     }
     "#,
  r#"import { template } from "@ember/template-compiler";
     function foo(this: void, message: string) {
       return function bar(this: void, message: string) {
         console.log(message);
       };
     }
     function makeTemplate() {
       return template(`hello`, { eval() { return eval(arguments[0]) } });
     }
     "#
}

main (test failed):

import { template } from "@ember/template-compiler";
function foo(this1: void, message1: string) {
  console.log(message1);
  return function bar1(this1: void, message1: string) {
    console.log(message1);
  };
}
function makeTemplate() {
  return template(`hello`, {
    eval () {
      return eval(arguments[0]);
    }
  });
}

this branch (test passed):

import { template } from "@ember/template-compiler";
function foo(this: void, message: string) {
  console.log(message);
  return function bar(this: void, message: string) {
    console.log(message);
  };
}
function makeTemplate() {
  return template(`hello`, {
    eval () {
      return eval(arguments[0]);
    }
  });
}

I don't actually understand either of these cases. Based on my understanding (which is evidently at least a bit wrong), I would expect something like:

import { template } from "@ember/template-compiler";
function foo(this: void, message: string) {
  console.log(message);
  return function bar(this: void, message1: string) {
    console.log(message1);
  };
}
function makeTemplate() {
  return template(`hello`, {
    eval () {
      return eval(arguments[0]);
    }
  });
}

Anyway, I think there is a sweet spot on upstream that doesn't break the import renaming but fixes the this param issue. If someone has time to bisect (or just try reverting to a point prior to the commit I pointed at), it would be great to get the .gts tests issue fixed while we work on #72.

[patricklx]

that still looks okay though. I'm more interested what would happen if a function parameter is named template in makeTemplate.
And what happens if no template is there? I think also on main, if no template is there, it will not rename any identifiers

[chancancode]

that still looks okay though.

Yes as far as correctness goes, it is fine, and in fact it is ultimately good that we don't touch an unrelated variable; the "I don't understand" part refers to the deviation between how I understood the hygiene system to work in swc compared to the generated output, which has some bearing to making sure we use it correctly internally, etc, but is not directly relevant to the end consumers.

I'm more interested what would happen if a function parameter is named template in makeTemplate.

That is the one test that is currently failing on this branch, and currently passes on main. My hypothesis (as I explained above) is that this commit may be the reason the test broke after the upgrade, and regardless, I hypothesize that there is probably a "sweet spot" in SWC that fixes the #71 (which can be confirmed with the unit test in my previous comment) but also didn't break the this test.

The task is therefore to find the commit by bisecting SWC, or just start by reverting to a65be14~ and run the existing tests PLUS the one I pasted above. In practice it's a bit more complicated than just git bisect or git revert, because we need to correctly rebase the changes on our fork with any changes in the upstream code; it is a bit tedious but doable.

And what happens if no template is there? I think also on main, if no template is there, it will not rename any identifiers

On main that relies on the hygiene code being a no-op if we didn't do anything. On my branch I made it explicitly not call the hygiene code unless we know we need to.

---

In the longer term @ef4 and I both believe that we should write our own code for this rather than continuing to use the SWC macro hygiene system, but until that can happen, what I am saying is there is probably a shorter path to fixing the immediate bug (by trail-and-error to find the sweet spot in SWC commits) that makes .gts tests a bit unusable at the moment.

[ef4]

Next action here: I want to add a test checking on the new collision detector because if I'm reading it correctly it detects a lot of collisions that aren't really collisions. This wouldn't result in incorrect code, but would result in unnecessary renaming, and much of the point of this code is to not have unnecessary renaming (we could have just used uuids for everything if we didn't care about that).

[chancancode]

Next action here: I want to add a test checking on the new collision detector because if I'm reading it correctly it detects a lot of collisions that aren't really collisions. This wouldn't result in incorrect code, but would result in unnecessary renaming, and much of the point of this code is to not have unnecessary renaming (we could have just used uuids for everything if we didn't care about that).

Yep more previous discussion https://discord.com/channels/480462759797063690/568935504288940056/1215336026374021180

[chancancode]

(But too be fair, not like swc’s code was great in that department either, so this basically makes the problem worse for template* variables but excluded the problem for any other variables. Definitely think we can do better though.)

[NullVoxPopuli]

I removed the merge of #74