Skip to content

Process a union or intersection type of all strings as a string - #675

Merged
Perryvw merged 1 commit into
TypeScriptToLua:masterfrom
martinjlowm:enhancement/detect-string-from-union
Jul 22, 2019
Merged

Process a union or intersection type of all strings as a string#675
Perryvw merged 1 commit into
TypeScriptToLua:masterfrom
martinjlowm:enhancement/detect-string-from-union

Conversation

@martinjlowm

@martinjlowm martinjlowm commented Jul 20, 2019

Copy link
Copy Markdown
Contributor

Prior to this PR, the following snippet would fail to transpile toUpperCase to upper.

Now, all subtypes of a union is checked and the following is transpiled from:

const union: string = "hello test";
if (union === 'hello' || union === 'test' || union === 'hello test') {
    return union.toUpperCase();
}

to

local union = "hello test"
if (union == 'hello' or union == 'test' or union == 'hello test') then
    return string.upper(union)
end

Comment thread src/LuaTransformer.ts Outdated
Comment thread src/LuaTransformer.ts Outdated
Comment thread test/unit/string.spec.ts Outdated
expect(result).toBe(inp.toUpperCase());
});

test.each(["hello", "test", "hello test"])("string.toUpperCase (union) (%p)", inp => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This test isn't really related to string.toUpperCase, but to general array string detection, so it should be be placed before string constrained generic foreach (%p) and be renamed
  2. There's no need for test.each, since it's not the main reason for this test
  3. The code inside template is indented with 2 spaces instead of 4
  4. Variables shouldn't begin with underscore, typescript uses it for it's unused diagnostics
  5. I don't see much reason to over-complicate test code with inference logic, something like this would be enough to prove test's point:
const union: "foo" | "bar" = "foo";
return union.length;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I was mainly concerned with string.* being transpiled correctly, hence I thought it made sense to have it around here. I'll move it :)
  2. I guess you are right.
  3. Woopsie 👍
  4. Gotcha - fixed in the next push.
  5. Your proposal doesn't cover this. union here is just a string literal which already supports .length. Enforcing union to be a string type triggers the type guard which appears to compare the string to a union of each or-condition. Inside the if-block, union is now inferred as an actual union of strings.
    const union: string = "foo";
    
    if (union === "foo" || union === "bar") {
        return union.length;
    }
    
    return 0;
    
    ^ does the trick, though!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, looks like TS ignores type annotation in this case and only something like const union = "foo" as "foo" | "bar"; works

@martinjlowm
martinjlowm force-pushed the enhancement/detect-string-from-union branch from 9ca89e5 to 9da0d68 Compare July 21, 2019 11:37
@martinjlowm

martinjlowm commented Jul 21, 2019

Copy link
Copy Markdown
Contributor Author

I've just pushed new changes based off of your feedback - thanks :)

Perhaps the test could have a better description, I'm open to suggestions.

Comment thread test/unit/string.spec.ts Outdated
}
);

test.each(["foo"])("scoped string-union inference (%p)", inp => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.each with a single case can be removed completely and be inlined to code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

@martinjlowm
martinjlowm force-pushed the enhancement/detect-string-from-union branch from 9da0d68 to 3b284fe Compare July 21, 2019 19:48
@Perryvw
Perryvw merged commit 6b9aeec into TypeScriptToLua:master Jul 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants