User defined imports name mangling #1865

infomiho · 2024-03-07T19:16:17Z

I've had some issues with conflicting imports when integrating Arctic #1851 and implemented this mangling to help.

Closes #1062
Closes #284

sodic

We already do the same thing in other places.

For example, these two:

wasp/waspc/src/Wasp/Generator/SdkGenerator/Server/OperationsGenerator.hs

Line 146 in 8d7c64a

. applyExtImportAlias

wasp/waspc/src/Wasp/Generator/ServerGenerator/ApiRoutesG.hs

Line 56 in 8d7c64a

"namespaceMiddlewareConfigFnImportAlias" .= middlewareConfigFnAlias

And possibly others.

I suggest a regex text search in the generated project). I used this command to find the ones I listed here (but didn't do a fully thorough check):

rg 'import.*\{.*as.*\}' -g '**/src/**/*' -g '**/sdk/**/*' -g '!**/dist/**/*'

Ideally, we'd have a single consistent procedure for handling all aliased imports. If not, we should at least make sure we don't apply two procedures for the same symbols (but I'd push for the "ideally" :)).

sodic · 2024-03-08T15:07:49Z

waspc/src/Wasp/Generator/JsImport.hs

+        mangleImportIdentifier JI.JsImport {JI._name = JsImportField name} = prefixTheImportName name jsImport
+
+        prefixTheImportName :: String -> JsImport -> JsImport
+        prefixTheImportName originalName = applyJsImportAlias (Just ("__userDefined" ++ toUpperFirst originalName))


I wanted to suggest mangling our imports instead of user imports (to keep the path between user symbols and our code as clear as possible).

However, it seems that it's not our imports conflicting with user imports - it's the thing we're defining in the same file.

Anyway, I don't how to make this without changing user symbol names, but am leaving this comment in case you have an idea.

sodic · 2024-03-08T15:20:24Z

Btw, your PR description (as its written now) won't trigger automatic closing of both issues. I got burned by this several times.

Instead of:

Closes #123 and #234

Do

Closes #123
Closes #234

Context: nus-cs2103-AY2021S2/forum#293

infomiho · 2024-03-08T16:17:12Z

Okay, so we need to unify the way we mangle the imports. I found the following using your regex:

sdk/wasp/server/operations/actions/index.ts and sdk/wasp/server/operations/queries/index.ts
Found in OperationsGenerator -> ("_ext" suffix using applyExtImportAlias and extImportToJsImport 🟣)
sdk/wasp/server/webSocket/index.ts
Found in WebSocketGenerator using the same fn as OperationsGenerator 🟣
server/src/routes/apis/index.ts
Found in ApiRoutesG (custom "wasp_" prefix 🔵)
server/src/middleware/globalMiddleware.ts
Found in ServerGenerator (custom "_waspGlobalMiddlewareConfigFn" 🔵)
server/src/jobs/*.ts
Found in JobGenerator (custom "_waspJobDefinition" 🔵)

🔵 manual alias
🟣 using the applyExtImportAlias and extImportToJsImport

Okay, that seems manageable. Great call to ripgrep!

infomiho · 2024-03-11T19:06:49Z

The biggest issue I'd like to resolve to make this "proper name mangling" is to somehow keep track of aliases that were used and ensure no duplicate aliases are used in the same file (thus breaking our code!).

Two cases

Happy path (works now)

Let simplest thing we can have in a file:

import { foo as foo__userDefined } from ...
import { bar as bar__userDefined from ...

This is all fine and in the basic case of nothing goes wrong since there are no duplicate import names i.e. we have foo and bar here.

Duplicate names

What if we had a situation like this in a single template file:

import { foo as foo__userDefined } from ...
import { foo as foo__userDefined } from ...

We now have duplicate names! foo and foo are both in the same file. This gives us foo__userDefined and foo__userDefined aliases. This would cause our code to break ⚠️

Solutions

Ideally, we would do it like e.g. Rollup does it and keep a global store of aliases and then on duplicate, we would just add a number at the end.

Ideal solution (keep track at template level)

If we kept the imports unique per-file, it might look like this:

File1:

import { foo as foo__userDefined$0 } from ...
import { foo as foo__userDefined$1 } from ...

File2:

import { foo as foo__userDefined$0 } from ...
import { foo as foo__userDefined$1 } from ...

Okay solution IMHO (keep track at global level)

Same like the previous solution, but we could have a "global state" and no per-file scoping:

File1:

import { foo as foo__userDefined$0 } from ...
import { foo as foo__userDefined$1 } from ...

File2:

import { foo as foo__userDefined$2 } from ...
import { foo as foo__userDefined$3 } from ...

infomiho · 2024-03-25T14:00:19Z

waspc/data/Generator/templates/server/src/dbSeed.ts

 {=/ dbSeeds =}

 const seeds = {
  {=# dbSeeds =}
-  {= importIdentifier =},
+  {= name =}: {= seedFn.importIdentifier =},


Since we reference the import name in the CLI, we had to adjust the code a bit.

infomiho · 2024-03-25T14:00:40Z

waspc/data/Generator/templates/server/src/middleware/globalMiddleware.ts

 {=/ globalMiddlewareConfigFn.isDefined =}
 {=^ globalMiddlewareConfigFn.isDefined =}
-const {=& globalMiddlewareConfigFn.importAlias =} = (mc: MiddlewareConfig) => mc


No need for a custom alias anymore 😄

infomiho · 2024-03-25T14:01:05Z

waspc/data/Generator/templates/server/src/routes/apis/index.ts

 {=# routeMiddlewareConfigFn.isDefined =}
 {=& routeMiddlewareConfigFn.importStatement =}
 {=/ routeMiddlewareConfigFn.isDefined =}
 {=/ apiRoutes =}

 const idFn: MiddlewareConfigFn = x => x

-{=# apiRoutes =}
-{=^ routeMiddlewareConfigFn.isDefined =}
-const {=& routeMiddlewareConfigFn.importAlias =} = idFn


No need for custom aliases anymore, rewrote this code to work with the new mangling.

infomiho · 2024-03-25T14:02:18Z

All of the code that needed name mangling needed to come inside of the Generator monad, which wasn't too much of a difference since we used the JsImport in FileDrafts anyways.

Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>

Martinsos · 2024-03-26T00:54:32Z

@infomiho I am curious, when does this situation where two user-defined symbols come into conflict happen? Does it happen if they have a query and action with the same name, or something like that? I am asking, because I wonder if we are even ok with allowing them to do something like that. But I am guessing there is some other use case you were considering here.

Because for me, the main thing I wanted us to fix with name mangling was ensuring the user-defined symbols don't come into conflict with the symbols that we defined.

Martinsos

Awesome initiative @infomiho !
I like the general idea, although it seems to me like we should still figure out some details a bit: I left comments.

I didn't review all the single files in the Generator where names are defined, but I reviewed the files with the core name mangling logic.

Martinsos · 2024-03-26T01:23:44Z

waspc/src/Wasp/Generator/JsImport.hs

+        mangleImportIdentifier :: JsImport -> Generator JsImport
+        mangleImportIdentifier JI.JsImport {JI._name = JsImportModule name} = mangleJsImportName name jsImport
+        mangleImportIdentifier JI.JsImport {JI._name = JsImportField name} = mangleJsImportName name jsImport
+
+        mangleJsImportName :: String -> JsImport -> Generator JsImport
+        mangleJsImportName originalName originalJsImport = do
+          mangledName <- mangleName originalName
+
+          return $ applyJsImportAlias (Just mangledName) originalJsImport


This split is a bit weird.

Your second function takes String + JsImport that has that same String in it, only to apply it as an alias.

I would combine these two into one function, it will be simpler to understand. Function could be defined as setMangledAlias :: JsImport -> Generator JsImport.
It also makes it clear that if there is an alias already, it will overwrite it. Which is behaviour you have right now, but I wonder if we are ok with that? Might be unexpected! If we are ok with it, I would make that clear.

And for getting the name out of JsImport -> you can have a case inside your function to do that easily:

let name = case jsImport of JI.JsImport { JI._name = JsImportModule name } -> name JI.JsImpoort { JI._name = JsImportField name } -> name

And then you can do mangledName <- mangleName name in the next line, and so o.

Or, what might be even better, is defining a standalone, top level, function called getJsImportNameSymbol :: JsImport -> String that gets you that name. Maybe there is a better more semantically correct name, you will know that better than me since this is fresher for you.

Martinsos · 2024-03-26T01:29:03Z

waspc/src/Wasp/Generator/JsImport.hs

@@ -36,16 +39,28 @@ extImportNameToJsImportName :: EI.ExtImportName -> JsImportName
 extImportNameToJsImportName (EI.ExtImportModule name) = JsImportModule name
 extImportNameToJsImportName (EI.ExtImportField name) = JsImportField name

-jsImportToImportJson :: Maybe JsImport -> Aeson.Value
+jsImportToImportJson :: Maybe JsImport -> Generator Aeson.Value


Ok, so so far, this function was, given a JsImport, returning a Json object that contains information about that import that we can easily use in a template.

I see two things immediately that are a bit suspicious:

Name is a bit confusing, feels like it could tell us more about what it does, or at least there could be a comment clarifying this.

I see it receives a Maybe JsImport, which is a bit weird, as I would usually expect such cases to be handled by the outside code. It is basically two functions in one -> returns Json for no import, or json for import. Which by clean code you might want to handle as two functions then. THat said, if it is really useful to us in practice, then fine, no worries, let's leave it as it is.

Ok, that said, let's focus on new changes. THis is now doing mangling also! If so, I find that a bit unexpected: I am turning jsImport into json, and now it also gets mangled. So maybe I would add that to the name, to make that obvious?
Or, what sounds like the best option to me, is to not do the mangling here, but to do it separately: either at the moment of constructing the JsImport itself, or as a separate function mangleJsImport :: JsImport -> JsImport. Separate function actually sounds the best. Then we have control over what we want to mangle vs what we don't want to mangle.

But I am guessing you might have a specific reason why you put it here? Because we want to always mangle everything?

Martinsos · 2024-03-26T01:33:29Z