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

Different comment values in TSC CLI and test file, Which one should We trust? #1005

Closed
sunrabbit123 opened this issue Apr 21, 2023 · 14 comments

Comments

@sunrabbit123
Copy link
Collaborator

class C {
  private p: string;
}

var str: string;
var bool: boolean;
var num: number;
var strOrNum: string | number;
var strOrBool: string | boolean;
var numOrBool: number | boolean;
var strOrNumOrBool: string | number | boolean;
var strOrC: string | C;
var numOrC: number | C;
var boolOrC: boolean | C;
var emptyObj: {};
var c: C;

// A type guard of the form typeof x === s,
// where s is a string literal with any value but 'string', 'number' or 'boolean',
//  - when true, removes the primitive types string, number, and boolean from the type of x, or
//  - when false, has no effect on the type of x.

if (typeof strOrC === "Object") {
  c = strOrC; // C
} else {
  var r2: string = strOrC; // string
}
if (typeof numOrC === "Object") {
  c = numOrC; // C
} else {
  var r3: number = numOrC; // number
}
if (typeof boolOrC === "Object") {
  c = boolOrC; // C
} else {
  var r4: boolean = boolOrC; // boolean
}
if (typeof strOrC === ("Object" as string)) {
  // comparison is OK with cast
  c = strOrC; // error: but no narrowing to C
} else {
  var r5: string = strOrC; // error: no narrowing to string
}

if (typeof strOrNumOrBool === "Object") {
  let q1: {} = strOrNumOrBool; // {}
} else {
  let q2: string | number | boolean = strOrNumOrBool; // string | number | boolean
}

what is the value of strOrC at the var r2: string = strOrC

@kdy1 kdy1 added this to the v0.0.1: Correctness milestone May 6, 2023
@kdy1
Copy link
Member

kdy1 commented May 6, 2023

I think strOrC should be never or string | C intuitively, but tsc may behave differently

@sunrabbit123
Copy link
Collaborator Author

In the absence of an actual_ty, TSC will not be a type_guard in an else statement.
However, comment wants to be a type_guard.
Feeding it a NotObject will fix it, but I'm not sure what's a good direction.

https://github.com/dudykr/stc/blob/main/crates/stc_ts_type_checker/tests/conformance/expressions/typeGuards/typeGuardOfFormTypeOfOther.error-diff.json
If you want it to be string | C, then your error-diff.json needs to be modified.

@sunrabbit123
Copy link
Collaborator Author

@kdy1

If you're right, and if TSC's findings are correct, the error is not an extra error, but it is being labeled as an extra error(Line 9).


if (typeof strOrC === "Object") {
c = strOrC; // C
}
else {
var r2: string = strOrC; // string
}

@kdy1
Copy link
Member

kdy1 commented Jun 2, 2023

I'm considering using the result of tsc cli instead of official test reference, because that's how tsc actually behave. @sunrabbit123 What do you think about this?

@kdy1
Copy link
Member

kdy1 commented Jun 2, 2023

It will be similar to #1040
If we decide to go with the CLI, tsc test suite will invoke tsc on the first run. (first run only because tsc is super slow)

@sunrabbit123
Copy link
Collaborator Author

@kdy1

I think so, too
Becasue I trust tsc cli feature, but I don't think apply every update

Unfortunately, the only official specification is tsc cli

Also, if the specification of tsc cli is changed, it will be casue that a lot of code will get type error
That's why I think we can trust tsc cli


ps. We have some additional extra errors that occurred for similar reasons
I would like to ask you how you intend to approach this work

@sunrabbit123
Copy link
Collaborator Author

Same issue #413
#413 (comment)

@sunrabbit123 sunrabbit123 changed the title Ambiguity in test results : expressions/typeGuards/typeGuardOfFormTypeOfOther.ts Different comment values in TSC CLI and test file, Which one should We trust? Jun 7, 2023
@ankhzet
Copy link

ankhzet commented Jun 29, 2023

A bit confusing type check here...
typeof xyz === "Object" should be lowercase 'object' (if stc implementation is case-insensitive, it might and probably would cause bugs in the consumer code):

> typeof {}
< 'object'
> typeof {} === 'Object'
< false

I suppose, it's from theese test cases?
https://github.com/dudykr/stc/blob/693cf5a891c5580542811b906616f0c15d0dd0fc/crates/stc_ts_type_checker/tests/conformance/expressions/typeGuards/typeGuardOfFormTypeOfOther.ts
https://github.com/dudykr/stc/blob/693cf5a891c5580542811b906616f0c15d0dd0fc/crates/stc_ts_type_checker/tests/conformance/expressions/typeGuards/typeGuardOfFormTypeOfNotEqualHasNoEffect.ts

@sunrabbit123
Copy link
Collaborator Author

@ankhzet
this issue mean next case

if (typeof strOrC === "Object") {
c = strOrC; // C
}
else {
var r2: string = strOrC; // string
}

this code should not work type guard, but comment mean work type guard
of course, tsc not work type guard

The comment is also attached to the recent typescript test code.

@ankhzet
Copy link

ankhzet commented Aug 1, 2023

Ok, got it.
Found the relevant PR microsoft/TypeScript#5442
Seems, narrowing to 'string' on the line 25 is intentional, invalidity of the typeguard is solved by the union intersection check itself failing with TS2367 error, as far as i understand.

@sunrabbit123
Copy link
Collaborator Author

The essence of this issue is what to do when the results of the TSC differ from the test annotations.

In fact, line 25 is inferred to be string | C.
But the code comment is written as string.

 if (typeof strOrC === "Object") { 
     c = strOrC; // C 
 } 
 else { 
     strOrC; // expected string, but this type is string | C
     var r2: string = strOrC;
 } 

playground

@ankhzet
Copy link

ankhzet commented Aug 1, 2023

As far as i understand, the test was written when strictNullChecks was not yet introduced (TS v2.0). If you test the code declaring strOrC as declare var strOrC: string | C; or initialize it, f.e. var strOrC: string | C = undefined as any;, then strOrC would be narrowed to string (this is how i tested the code in the playground).
Alternatively you can disable the flag in the playground, so it behaves the same way it was at the time when the test case was introduced: playground - strOrC is narrowed to string, no error.

So the issue might be a discrepancy between the tsconfig used for reference tests execution ty the TS team and the default tsc/playground config you usually use for testing, i guess.

@sunrabbit123
Copy link
Collaborator Author

omg, thanks your report
I missing important information about options

Issue has been created in relation to it.

@sunrabbit123
Copy link
Collaborator Author

The reason for the discussion of the issue is deemed to have disappeared, so we close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants