Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow negative numbers in create numeric literal (take 2) #56570

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Nov 28, 2023

reverts #56422 which was a revert of #55268 but this version now focuses on negative numbers alone - without making any changes to Infinity (and that was the only reason why the original PR was reverted), cc @jakebailey

this would prevent issues like #56562 😉

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 28, 2023
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@jakebailey
Copy link
Member

This looks good to me besides the test diff.

IIRC our intention was to investigate actually adding an infinity/nan type, so we're leaving the status quo on 1e999 and -1e999 for now. I believe this PR doesn't affect that.

@Andarist
Copy link
Contributor Author

Andarist commented Dec 6, 2023

IIRC our intention was to investigate actually adding an infinity/nan type, so we're leaving the status quo on 1e999 and -1e999 for now. I believe this PR doesn't affect that.

I can confirm that's the intention here. Only negative finite numbers should be affected by this change, and it just forbids creating "wrong" AST nodes like that.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Actually, one suggestion to directly negate instead of using abs; this matches another change made in #56565 (which I am going to merge first, funnily your code would have caught this bug albeit without a test).

src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/transformers/declarations.ts Outdated Show resolved Hide resolved
src/compiler/transformers/ts.ts Outdated Show resolved Hide resolved
@Andarist
Copy link
Contributor Author

Andarist commented Dec 7, 2023

this matches another change made in #56565 (which I am going to merge first, funnily your code would have caught this bug albeit without a test).

Yeah, that PR reminded me that I was supposed to re-propose this change :)

Actually, one suggestion to directly negate instead of using abs;

done

@jakebailey jakebailey merged commit 96bef67 into microsoft:main Dec 7, 2023
19 checks passed
c0sta pushed a commit to c0sta/TypeScript that referenced this pull request Dec 20, 2023
raurfang added a commit to raurfang/openapi-typescript that referenced this pull request Mar 30, 2024
- append a `ValueMinus` string to numeric enum member names, if a name is not explicitly provided.
- create negative numeric enum member values using `ts.factory.createPrefixUnaryExpression()`

Previous behavior:

When there is no name explicitly provided for them, numeric enum member names are
constructed by prefixing the number with a `Value` string.
The code did not take negative numbers into account, which resulted in enum members like:
`_100 = -100`.

Additionally, since Typescript 5.4 (see microsoft/TypeScript#56570),
the `tsEnumMember` function failes on negative enum members.
drwpow pushed a commit to openapi-ts/openapi-typescript that referenced this pull request Mar 30, 2024
- append a `ValueMinus` string to numeric enum member names, if a name is not explicitly provided.
- create negative numeric enum member values using `ts.factory.createPrefixUnaryExpression()`

Previous behavior:

When there is no name explicitly provided for them, numeric enum member names are
constructed by prefixing the number with a `Value` string.
The code did not take negative numbers into account, which resulted in enum members like:
`_100 = -100`.

Additionally, since Typescript 5.4 (see microsoft/TypeScript#56570),
the `tsEnumMember` function failes on negative enum members.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants