Add preliminary support for Mapped Types #122

roboz0r · 2024-09-15T14:07:20Z

declare const NODES: readonly ["a", "button"];
type Primitives = {
    [E in (typeof NODES)[number]]: boolean;
};

transforms to

[<AllowNullLiteral>]
[<Interface>]
type Primitives =
    abstract member a: bool with get, set
    abstract member button: bool with get, set

Summary

Adds new GlueType.MappedType
Adds ObjectType to IndexedAccessType
Adds corresponding transform functions
Adds SyntaxKind.name helper function for debugging
Adds __SOURCE_FILE__ literals to errors for debugging

[119]

[converter]

MangelMaxime

Thank you for start the work.

I clean up a little the code, made it more type safe but I think overall it looks fine.

I will also have a look to improve the error message because now with the change introduced in this PR, they are not all on par..

src/Glutinum.Converter/Reader/MappedTypeNode.fs

src/Glutinum.Converter/Transform.fs

src/Glutinum.Converter/GlueAST.fs

roboz0r · 2024-09-15T16:14:58Z

I will also have a look to improve the error message because now with the change introduced in this PR, they are not all on par..

If you're happy with my changes to the error reporting I'll go through and add SyntaxKind.name and __SOURCE_FILE__ into the relevant locations. I was primarily thinking of myself when running the tests and going back and forth between the enum list and trying to work out the file where the error originated.

[119]

roboz0r · 2024-09-15T17:40:39Z

I added SyntaxKind.name in the relevant error messages that I found, I also added __SOURCE_FILE__ to errors that aren't throwing exceptions as the exception would contain source information anyway.

MangelMaxime · 2024-09-15T18:41:51Z

I was primarily thinking of myself when running the tests and going back and forth between the enum list and trying to work out the file where the error originated.

My way of doing it was by copy / searching the context error across the file. But having the name of the source file, is either because in general editors allow you to fuzzy search by filename in general.

It is just a shame that we can't do something like:

module Log =

    let inline info (message: string) =
        printfn $"%s{__SOURCE_FILE__}: %s{message}"

Where __SOURCE_FILE__ is from the caller thanks to inlining the function

…r to read [converter]

MangelMaxime · 2024-09-15T18:54:32Z

src/Glutinum.Converter/Reader/TypeNode.fs

+let private readTypeQuery
+    (reader: ITypeScriptReader)
+    (typeNodeQuery: Ts.TypeQueryNode)
+    =
+    let checker = reader.checker
+    let typ = checker.getTypeAtLocation !!typeNodeQuery.exprName
+
+    resultOption {
+        // This is safe as both cases have a `kind` field
+        let exprNameKind: Ts.SyntaxKind = typeNodeQuery.exprName?kind
+        // This logic is highly overlapping with readTypeUsingFlags
+        // It should be refactored to achieve a sane flow
+        match typ.flags, typ.getSymbol (), exprNameKind with
+        | HasTypeFlags Ts.TypeFlags.Object, None, Ts.SyntaxKind.Identifier ->
+            let exprName: Ts.Identifier = !!typeNodeQuery.exprName
+
+            let! aliasSymbol =
+                checker.getSymbolAtLocation exprName
+                |> ResultOption.ofOption
+                |> Result.mapError (fun _ ->
+                    generateReaderError
+                        "type node (TypeQuery)"
+                        "Missing symbol"
+                        typeNodeQuery
+                )
+
+            let! declaration =
+                aliasSymbol.declarations
+                |> Option.map (fun declarations ->
+                    if declarations.Count <> 1 then
+                        None
+                    else
+                        Some declarations.[0]
+                )
+                |> ResultOption.ofOption
+                |> Result.mapError (fun _ ->
+                    generateReaderError
+                        "type node (TypeQuery)"
+                        "Expected exactly one declaration"
+                        typeNodeQuery
+                )
+
+            let! variableDeclaration =
+                declaration
+                |> Option.bind (fun declaration ->
+                    match declaration.kind with
+                    | Ts.SyntaxKind.VariableDeclaration ->
+                        Some(declaration :?> Ts.VariableDeclaration)
+                    | _ -> None
+                )
+                |> ResultOption.ofOption
+                |> Result.mapError (fun _ ->
+                    generateReaderError
+                        "type node (TypeQuery)"
+                        "Unsupported declaration kind"
+                        typeNodeQuery
+                )
+
+            let! typeNode =
+                variableDeclaration.``type``
+                |> ResultOption.ofOption
+                |> Result.mapError (fun _ ->
+                    generateReaderError
+                        "type node (TypeQuery)"
+                        "Missing type"
+                        typeNodeQuery
+                )
+
+            match typeNode.kind with
+            | Ts.SyntaxKind.TypeOperator ->
+                let typeOperatorNode = typeNode :?> Ts.TypeOperatorNode
+
+                return reader.ReadTypeOperatorNode typeOperatorNode
+
+            | _ -> return readTypeUsingFlags reader typ
+
+        | _ -> return readTypeUsingFlags reader typ
+    }


@roboz0r

I gave another chance to rewrite the TypeQuery code. Do you think this version of the code is easier to read?

Personally, I think it is easier to read because it remove (at least visually) several level of pattern matching over options.

Happy to use the CE to flatten the nested pattern matching but I'd really like to be able to combine and refactor this function and readTypeUsingFlags to avoid the overlapping logical branches. I couldn't work out an easy way to do it yesterday but if you think it's a good idea too, I'll give it another go.

Maybe it should also be split off into a separate Reader/TypeQueryNode.fs file

Happy to use the CE to flatten the nested pattern matching but I'd really like to be able to combine and refactor this function and readTypeUsingFlags to avoid the overlapping logical branches. I couldn't work out an easy way to do it yesterday but if you think it's a good idea too, I'll give it another go.

I didn't look too much on that, but if you can refactor the code to have a single function/path I am all for it. With or without the CE depending on what works best.

@MangelMaxime I came up with an alternative rewrite of this code here 031dcd5 I found that just result {} rather than resultOption was sufficient.

I left a few comments on your commit but I think we are on the good track. If we fix the comments, we should be able to merge it in Glutinum and keep moving forward.

MangelMaxime · 2024-09-15T19:26:35Z

I also gave a chance to re-think the error reporter:

let generateReaderError
    (fileFolder: string, fileName: string, fileLine: string)
    (errorContext: string)
    (reason: string)
    (node: Ts.Node)
    =
    let sourceFile = node.getSourceFile ()
    let lineAndChar = sourceFile.getLineAndCharacterOfPosition node.pos
    let line = int lineAndChar.line + 1
    let column = int lineAndChar.character + 1

    let errorOrigin =
        let fileFolder =
            let index = fileFolder.IndexOf("src/Glutinum.Converter")
            "./" + fileFolder.Substring(index)

        $"%s{fileFolder}/%s{fileName}(%s{fileLine})"

    $"""%s{errorOrigin}: Error while reading %s{errorContext} from:
%s{sourceFile.fileName}(%d{line},%d{column})

%s{reason}

--- Text ---
%s{node.getFullText ()}
---

--- Parent text ---
%s{node.parent.getFullText ()}
---"""

Example of output:

./src/Glutinum.Converter/Reader/TypeNode.fs(184): Error while reading type node (TypeQuery) from:
/Users/mmangel/Workspaces/Github/glutinum-org/Glutinum.Converter/tests/specs/references/typeof/typeofStrings.d.ts(3,12)

Missing symbol

--- Text ---
typeof NODES
---

--- Parent text ---
 (typeof NODES)
---

This allows to ensure that all errors have the same level of information, because it can be easy to forget to add in {__SOURCE_FILE__}.

I decided to use a relative path for reporting the origin of the error because it seems like for VSCode it allows to click on the link to go the file declaration.

If others IDEs have don't support that, we can do something like:

    let errorOrigin =
        let fileFolder =
        #if DEBUG
            $"%s{fileFolder}"
        #else
            let index = fileFolder.IndexOf("src/Glutinum.Converter")
            "./" + fileFolder.Substring(index)
        #endif

like that in DEBUG we generate a full path and in release we have a relative path. The relative path in release, is because showing my system architecture doesn't matter and add noise for no reason.

What do you think? If you agree on this proposition, I can make the change to the PR

roboz0r · 2024-09-15T19:28:50Z

Where __SOURCE_FILE__ is from the caller thanks to inlining the function

In .NET land we could use [<CallerFilePath>] but I expect that doesn't work with Fable.

roboz0r · 2024-09-15T19:51:00Z

re-think the error reporter

proposed error format looks good. Just a note to normalize \ to / before processing the paths. Unfortunately it seems like the System.IO.Path methods haven't been ported over to Fable.

MangelMaxime · 2024-09-15T20:03:17Z

Where __SOURCE_FILE__ is from the caller thanks to inlining the function

In .NET land we could use [<CallerFilePath>] but I expect that doesn't work with Fable.

You would be surprised at how many times I have been surprised by Fable. And you can add +1 to that number because [< CallerFilePath>] works 🚀.

Thanks you for the tips, you have now idea for how long I have been looking for something like that 😅 (probably 5 years). It is really useful, when generating documentation with link to the source code.

I just need to rewrite the generateReaderError from a function to a member. I will probably create a Reporter type like that it will be able to host things like error, warning, info, etc. if we need to extends it later.

re-think the error reporter

proposed error format looks good. Just a note to normalize \ to / before processing the paths. Unfortunately it seems like the System.IO.Path methods haven't been ported over to Fable.

Nothing that a good old Replace can't fix 😅

MangelMaxime · 2024-09-16T18:44:49Z

@roboz0r This PR was starting to be a bit messy with the different changes between the Reader + logger improvements.

I think it is best if you can open a new PR using the latest version of main in which I added the improvements we talked regarding the logger.

roboz0r · 2024-09-17T01:44:20Z

New PR #124

roboz0r and others added 5 commits September 15, 2024 08:56

feat: Add preliminary support for mapped types

a489b7e

[119]

chore: Remove overlapped name

4766542

chore: little clean up

65f6704

refactor: use same coding style has the rest of the file

6686f2f

[converter]

refactor: make the code safer + better error reporting

3e5f401

[converter]

MangelMaxime requested changes Sep 15, 2024

View reviewed changes

src/Glutinum.Converter/Reader/MappedTypeNode.fs Outdated Show resolved Hide resolved

src/Glutinum.Converter/Transform.fs Outdated Show resolved Hide resolved

src/Glutinum.Converter/GlueAST.fs Outdated Show resolved Hide resolved

roboz0r added 3 commits September 15, 2024 11:38

refactor: MappedType name is "obj"

0119b29

[119]

refactor: Improved readMappedTypeNode error messages

69815b9

[119]

chore: More consistent error messages

c08da7a

roboz0r requested a review from MangelMaxime September 15, 2024 17:39

MangelMaxime force-pushed the mapped-types branch from d9622d8 to 0e50033 Compare September 15, 2024 18:50

refactor: rewrite handling of TypeQuery trying to make the code easie…

a5abdc3

…r to read [converter]

MangelMaxime force-pushed the mapped-types branch from 0e50033 to a5abdc3 Compare September 15, 2024 18:53

MangelMaxime reviewed Sep 15, 2024

View reviewed changes

MangelMaxime closed this Sep 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preliminary support for Mapped Types #122

Add preliminary support for Mapped Types #122

roboz0r commented Sep 15, 2024

MangelMaxime left a comment

roboz0r commented Sep 15, 2024

roboz0r commented Sep 15, 2024

MangelMaxime commented Sep 15, 2024

MangelMaxime Sep 15, 2024

roboz0r Sep 15, 2024 •

edited

Loading

MangelMaxime Sep 15, 2024

roboz0r Sep 15, 2024

MangelMaxime Sep 16, 2024

MangelMaxime commented Sep 15, 2024

roboz0r commented Sep 15, 2024

roboz0r commented Sep 15, 2024

MangelMaxime commented Sep 15, 2024

MangelMaxime commented Sep 16, 2024

roboz0r commented Sep 17, 2024

Add preliminary support for Mapped Types #122

Add preliminary support for Mapped Types #122

Conversation

roboz0r commented Sep 15, 2024

Summary

MangelMaxime left a comment

Choose a reason for hiding this comment

roboz0r commented Sep 15, 2024

roboz0r commented Sep 15, 2024

MangelMaxime commented Sep 15, 2024

MangelMaxime Sep 15, 2024

Choose a reason for hiding this comment

roboz0r Sep 15, 2024 • edited Loading

Choose a reason for hiding this comment

MangelMaxime Sep 15, 2024

Choose a reason for hiding this comment

roboz0r Sep 15, 2024

Choose a reason for hiding this comment

MangelMaxime Sep 16, 2024

Choose a reason for hiding this comment

MangelMaxime commented Sep 15, 2024

roboz0r commented Sep 15, 2024

roboz0r commented Sep 15, 2024

MangelMaxime commented Sep 15, 2024

MangelMaxime commented Sep 16, 2024

roboz0r commented Sep 17, 2024

roboz0r Sep 15, 2024 •

edited

Loading