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

Evaluate expressions with annotated constant variables #57236

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link
Contributor

fixes #57231

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jan 30, 2024
@DanielRosenwasser
Copy link
Member

Any clue why we disallowed annotated variables previously?

@ahejlsberg
Copy link
Member

Any clue why we disallowed annotated variables previously?

See #57199.

@ahejlsberg
Copy link
Member

This PR doesn't really fix the issue. See here.

@Andarist
Copy link
Contributor Author

Andarist commented Jan 30, 2024

Hm, ye - I assumed that the isDiscriminantProperty was intentionally not checking this kind of thing for performance reasons.

So this PR might not fix the referenced issue in full. Would it still make sense to have an adjusted version of it in? The mentioned enum case requires some extra test cases that would involve potential circularities but the template expression case isn't prone to that, right? The expression like this can't depend on itself (I'm sure that would already be reported as an error elsewhere 😅 )

I'm also not sure how the potential circularity is a big problem here. Circularties are already reported on many occasions. If the constant evaluation leads to a circularity shouldn't that just lead to reporting a circularity? The preemptive bailout from the evaluation is quite surprising to me here.

@ahejlsberg
Copy link
Member

ahejlsberg commented Jan 31, 2024

Would it still make sense to have an adjusted version of it in?

Consider declarations like the following:

const someValue: number = 42;

The intuition here is that someValue represents some number that I can change at will without affecting type checking because I have explicitly indicated I want it treated like it could be any number. That wouldn't be the case if constant evaluation "cheated" and looked at the value anyway.

Then what if the type annotation was "42", i.e. exactly a singleton literal type? Sure, we could consider allowing that, but not clear it adds a lot of value.

@Andarist
Copy link
Contributor Author

The intuition here is that someValue represents some number that I can change at will without affecting type checking because I have explicitly indicated I want it treated like it could be any number. That wouldn't be the case if constant evaluation "cheated" and looked at the value anyway.

That would be incorrect and it was a total oversight on my part. I somehow assumed that the type was already obtained in that conditional block and passing tests didn't make me reinspect it 😉

I pushed out the fix for this particular issue.

Then what if the type annotation was "42", i.e. exactly a singleton literal type? Sure, we could consider allowing that, but not clear it adds a lot of value.

I think that the main motivation behind this wouldn't be an annotation with a literal type but rather with a union type. The user might want to make sure that the initializer's type is assignable to some other type - that doesn't seem like an edge-case scenario. I think this should not affect the constant evaluation since it doesn't change the initial~ type of a variable.

Even when we consider the literal type annotation - it doesn't bring any value and can be seen as redundant I find it unexpected that it has an effect here. Its effect could be similar to a redundant parenthesized expression - it could just be "skippable". As is, it feels like it breaks the least surprise principle.

The union case might go slightly against your argument that an explicit indication should make it OK to change the initializer at will. However, I think that the existence of assignment-reduced types already trumps that. Initializers might already "leak" past the declared type.

Taking a closer look at constant expressions and declared types or type operators vs unions:

const v1 = "foo";
const test1 = `pre${v1}`; // "prefoo"

const v2 = "foo" as const;
const test2 = `pre${v2}`; // string

type Union = "foo" | "bar";

const v3 = "foo" satisfies Union;
const test3 = `pre${v3}`; // string

const v4: Union = "foo";
const test4 = `pre${v4}`; // string

We can see that the declared type situation - the one that I touched in this PR - is not that unique. However, I'd argue that all of them should work the same way and that they should return "prefoo" (can you get any more constant than v2? :P). I could make the required changes for this as part of this PR.

The examples here use template literal expressions but the same applies to enum members.

getAssignmentReducedType(declaredType as UnionType, getInitialType(declaration)) :
declaredType;
if (type.flags & TypeFlags.StringOrNumberLiteral) {
return (type as StringLiteralType | NumberLiteralType).value;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the initializer is still required for all of this to work here. I'm not sure what's the preferred way of handling potential errors in situations like this:

const str: 'foo' = 'bar'
const another = `_${str}` // "_foo" or "_bar" ?

I think the argument can be made both ways. I'm not sure if this matters all that much when the program errors anyway - but perhaps using the evaluated initializer in this situation would be slightly more consistent. After all, the initializer is still required for this to work - so the evaluation happens on the grounds that the expression that can be evaluated is there. It's not like this can be used as part of the evaluation: declare const str: 'foo';

It's also worth noting that this code here should be expanded with support for enum members.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Status: Waiting on reviewers
Development

Successfully merging this pull request may close these issues.

discriminated union type matching behaviour changed starting from 5.2.x
4 participants