Make tuple and custom array declared as interface compatible #413

devgony · 2022-12-18T07:27:54Z

Description: Make tuple and custom array declared as interface compatible

BREAKING CHANGE:

Related issue (if exists): To resolve #216

CLAassistant · 2022-12-18T07:27:58Z

All committers have signed the CLA.

devgony · 2022-12-18T07:30:43Z

crates/stc_ts_file_analyzer/src/analyzer/types/mod.rs

+                    for type_arg in extend.type_args.as_ref() {
+                        for param in type_arg.to_owned().params {
+                            match param {
+                                Type::Array(array) => return Ok(Cow::Owned(Type::Array(array))),


I tried early return here to see what makes difference but it does not affect the result of ./scripts/test.sh arityAndOrderCompatibility at all

Is ./scripts/test.sh arityAndOrderCompatibility correct to test?

You should add Type::Interface to

stc/crates/stc_ts_file_analyzer/src/analyzer/assign/mod.rs

Lines 512 to 521 in d6dc481

Type::Conditional(..)

| Type::IndexedAccessType(..)

| Type::Alias(..)

| Type::Instance(..)

| Type::Intrinsic(..)

| Type::Mapped(..)

| Type::Operator(Operator {

op: TsTypeOperatorOp::KeyOf,

..

}) => {

The command itself is correct

devgony · 2022-12-19T08:06:55Z

$ ./scripts/test.sh arityAndOrderCompatibility

2 unmatched errors out of 17 errors. Got 5 extra errors.
Wanted: [RefError { line: 19, column: 5, code: "TS2741" }, RefError { line: 22, column: 5, code: "TS2741" }]
Unwanted: [(19, "TS2322"), (22, "TS2322"), (30, "TS2322"), (34, "TS2322"), (35, "TS2322")]

I made it to remove (16, "TS2461").
Should i remove all the other 5 extra errors In this issue? 😢

kdy1 · 2022-12-19T09:05:31Z

No, it's okay to split PRs. I'll review this PR once you undraft it.

devgony · 2022-12-20T00:27:46Z

I'm trying to solve nested_assignment error in advance of undraft but cannot get which part is wrong yet.

devgony · 2022-12-21T03:58:17Z

crates/stc_ts_file_analyzer/src/analyzer/types/mod.rs

+            }) => {
+                for extend in extends {
+                    let types = extend.to_owned().type_args.into_iter().flat_map(|cur| cur.params);
+                    let union_type = Type::new_union(*span, types);


It almost solved nest_assignment errors but not RegExpExecArray yet.
Could you give me any hint where to get RegExpExecArray type to return Array?

RegExpExecArray is declared as

interface RegExpExecArray extends Array<string> { index: number; input: string; }

So you need to check the type arguments of Array

kdy1 · 2023-01-14T11:58:13Z

Can you sign the CLA?

kdy1

Thanks!

kdy1 · 2023-01-14T11:59:49Z

crates/stc_ts_file_analyzer/src/analyzer/types/mod.rs

+                for extend in extends {
+                    let types = extend.to_owned().type_args.into_iter().flat_map(|cur| cur.params);
+                    let union_type = Type::new_union(*span, types);
+                    if let box RExpr::Ident(ident) = &extend.expr {


I think we should check for Type::Array after using self.type_of_ts_entity_name

kdy1 · 2023-01-14T12:51:32Z

crates/stc_ts_file_analyzer/src/analyzer/types/mod.rs

@@ -117,6 +116,22 @@ impl Analyzer<'_, '_> {
            | Type::EnumVariant(..)
            | Type::Param(_)
            | Type::Module(_) => return Ok(ty),
+            Type::Interface(Interface {


I think this approach is wrong

Did you mean we should add more condition here to decrease the range of match arm?
Or It is totally wrong location to implement?

I meant the latter

Can you give any tips like where to start first?

I think some match expressions should be added to

stc/crates/stc_ts_file_analyzer/src/analyzer/assign/mod.rs

Line 1036 in 4033e8b

kdy1 · 2023-01-14T12:51:56Z

crates/stc_ts_file_analyzer/src/analyzer/types/mod.rs

+                tracker,
+            }) => {
+                for parent in extends {
+                    let parent = self.type_of_ts_entity_name(parent.span(), &parent.expr, parent.type_args.as_deref())?;


A user can add methods to their own array type.

And this does not handle tuple types correctly

…bility

…ibility

kdy1 · 2023-02-01T09:49:21Z

crates/stc_ts_file_analyzer/src/analyzer/assign/mod.rs

+                metadata: InterfaceMetadata { common },
+                tracker,
+            }) => {
+                for parent in extends {


I think this logic is too naive

kdy1 · 2023-02-01T09:49:51Z

crates/stc_ts_file_analyzer/src/analyzer/assign/mod.rs

+            }) => {
+                for parent in extends {
+                    let ty = self.type_of_ts_entity_name(parent.span(), &parent.expr, parent.type_args.as_deref())?;
+                    if let Type::Array(_) = &ty {


Checking for Type::Array is not enough, and we should handle this by accessing iterating over properties

interface MyTuple extends Array<unknown> { 0: number 1: number } declare let a: MyTuple declare let b: [number, number] a = b // ok b = a // error

https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgLIE8AqBXADgGxQgA9IQATAZ2QEEoo50AebEAaxAHsB3EAPmQBvAFDIxyAAwAuZCGwBbAEbRR4gIwy5SlQF9hw8hAT44UFITDI4MjDgIQDRk2eQXkimQG0tyqABpZBV8AXX04ZABed2QAehjkTjZhRUirWPjoKE4oIA

interface MyTuple extends Array<any> { 0: number 1: number } declare let a: MyTuple declare let b: [number, number] a = b // ok b = a // error

https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgLIE8AqBXADgGxQgA9IQATAZ2QEEoo50AeOEdAPmQG8AoZf5AAYAXMhDYAtgCNofAQEZR46bIC+PHuQgJ8cKCkJhkcURhwEIm7bv3JDyKaIDaymVAA0YyW4C6GuMgAvA7IAPShyAD2ANY8UkHGYRHQUJFQQA

As you can see, we can't rely on the type argument in extends Array<T>

devgony · 2023-02-07T04:16:16Z

crates/stc_ts_file_analyzer/src/analyzer/assign/mod.rs

+                                {
+                                    println!("@@@\n{lkind:#?}:{rkind:#?}");
+                                    let diff = lkind == rkind;
+                                    println!("@@@\n{diff}");


Is there any existing function to compare two KeywordType and return reasonable error?

Do you think I'm going correct way?

You can use assign_with_opts(...)? to do it

I think you should not reimplement whole logic here, but delegate to a recursive call to assign_with_opts

It looks like we should keep logic here.
Otherwise, it breaks other tests
https://github.com/dudykr/stc/actions/runs/4110781244/jobs/7093929846

kdy1 · 2023-06-03T11:56:41Z

Adding more tests would be awsome. Thank you!

kdy1 · 2023-06-03T11:56:47Z

Adding more tests would be awesome. Thank you!

… arityAndOrderCompatibility

devgony · 2023-06-03T14:15:59Z

crates/stc_ts_type_checker/tests/conformance/types/tuple/arityAndOrderCompatibility01.ts

+// x = z; // should get TS2322 but pass
+y = x;
+y = z;
+// z = x; // should pass but got TS2322


According to TS Playground,
It seems that x = z and z = x work in opposite directions.
Should I follow TS playground and fix it in this PR?

TSPlayground

I think that this issue is related to the following issue @kdy1
#1005

@sunrabbit123
Thanks for the information.
So it looks like it does not need to be fixed, at least in this PR.

z = x must be not emit 2322
x = z must be emit 2322

I think the code is fine.

And it seems to be out of the conventional test case.

When I commented above, I thought it was an existing test case.
I'm sorry.

kdy1 · 2023-06-06T09:57:51Z

crates/stc_ts_type_checker/tests/tsc-stats.rust-debug

+    required_error: 874,
+    matched_error: 1749,
+    extra_error: 284,
+    panic: 20,


Can you run check.sh agian?

I got thread 'conformance::types::uniqueSymbol::uniqueSymbolsDeclarations.ts' has overflowed its stack
Should i pass some option to avoid the overflow?

The way is simple

Check that the code you wrote affects stackoverflow.

If it does, insert some stackguards.

if it doesn't, it's broken and you can report it in an issue

Thanks you

The following code detects the overflow and handles it.

let _stack = match stack::track(actual_span) { Ok(v) => v, Err(err) => { // print_backtrace(); return Err(err.into()); } };

…ibility

changeset-bot · 2023-12-29T04:18:29Z

⚠️ No Changeset found

Latest commit: ed10347

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

kdy1 · 2023-12-29T04:45:19Z

crates/stc_ts_type_checker/tests/conformance/types/tuple/arityAndOrderCompatibility01.ts

@@ -33,3 +33,8 @@ var n3: [number, string] = z;
 var o1: [string, number] = x;
 var o2: [string, number] = y;
 var o3: [string, number] = y;
+
+x = y;


Please revert this

This test file is copied from the official tsc conformance test suite, so we should not modify it.

kdy1

It seems like this PR does not improve the test stats?

devgony commented Dec 18, 2022

View reviewed changes

kdy1 added this to the 1. No false positive milestone Dec 18, 2022

kdy1 self-assigned this Dec 18, 2022

devgony commented Dec 21, 2022

View reviewed changes

kdy1 removed their assignment Jan 10, 2023

devgony marked this pull request as ready for review January 14, 2023 11:22

feat: implement interface array case

370071b

devgony force-pushed the arityAndOrderCompatibility branch from 4eecbee to 370071b Compare January 14, 2023 11:53

kdy1 self-assigned this Jan 14, 2023

kdy1 requested changes Jan 14, 2023

View reviewed changes

fix: filter array by type_of_ts_entity_name

ce4cf98

kdy1 requested changes Jan 14, 2023

View reviewed changes

kdy1 removed their assignment Jan 15, 2023

devgony added 5 commits February 1, 2023 17:12

temp-1

fc48981

fix: implement handling interface at assign/mod.rs

71cefdd

Merge branch 'arityAndOrderCompatibility-n' into arityAndOrderCompati…

0399886

…bility

fix: remove previous implement

96f4567

fix: remove imports

f27ad7e

devgony requested a review from kdy1 February 1, 2023 09:05

devgony added 2 commits February 1, 2023 18:07

chore: remove redundant diff

43f4aec

Merge remote-tracking branch 'upstream/main' into arityAndOrderCompat…

3194496

…ibility

kdy1 requested changes Feb 1, 2023

View reviewed changes

devgony added 2 commits February 7, 2023 08:20

fix: compare types of lhs and rhs

20f4513

fix: zip and iterate each lkind and rkind

bd11c05

devgony commented Feb 7, 2023

View reviewed changes

devgony added 4 commits June 3, 2023 21:56

test: add a case that should pass

4976748

fix: replace the order of lhs and rhs

3d3fcca

Merge remote-tracking branch 'origin/arityAndOrderCompatibility' into…

e85466f

… arityAndOrderCompatibility

feat: correct errors.json

e4a786b

devgony commented Jun 3, 2023

View reviewed changes

devgony added 2 commits June 3, 2023 23:54

refactor: remove redundant line, reduce clone

fb8d765

fix: renamt lhss to lsh_types

0a08ed7

kdy1 reviewed Jun 6, 2023

View reviewed changes

devgony added 3 commits June 6, 2023 22:49

fix: remove unresolved test cases

bdf5da0

chore: run check.sh

5759dc2

fix: correct error line number

0da1d65

sunrabbit123 mentioned this pull request Jun 7, 2023

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

Closed

Merge remote-tracking branch 'upstream/main' into arityAndOrderCompat…

594c1e1

…ibility

devgony requested a review from kdy1 December 29, 2023 04:30

kdy1 requested changes Dec 29, 2023

View reviewed changes

devgony added 4 commits December 29, 2023 13:57

fix: recover official tsc conformance test suite

4a839ca

fix: remove error on line 39

6ba341b

fix: amend matched_error to 17

3df5f8b

fix: remove formatting

b8551db

devgony requested a review from kdy1 December 29, 2023 06:02

kdy1 reviewed Dec 29, 2023

View reviewed changes

devgony added 5 commits January 25, 2024 23:24

fix: recover solved issue

3ea7e93

feat: handle line16

5f8c1df

feat: handle line17, 19

97256d8

test: add arityAndOrderCompatibility02 case

66ecbd5

chore: add test log

ed10347

devgony requested a review from kdy1 January 26, 2024 08:04

	Type::Conditional(..)
	\| Type::IndexedAccessType(..)
	\| Type::Alias(..)
	\| Type::Instance(..)
	\| Type::Intrinsic(..)
	\| Type::Mapped(..)
	\| Type::Operator(Operator {
	op: TsTypeOperatorOp::KeyOf,
	..
	}) => {

Make tuple and custom array declared as interface compatible #413

Are you sure you want to change the base?

Make tuple and custom array declared as interface compatible #413

Conversation

devgony commented Dec 18, 2022

CLAassistant commented Dec 18, 2022 • edited Loading

devgony Dec 18, 2022 • edited Loading

Choose a reason for hiding this comment

kdy1 Dec 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devgony commented Dec 19, 2022

kdy1 commented Dec 19, 2022

devgony commented Dec 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdy1 commented Jan 14, 2023

kdy1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdy1 commented Jun 3, 2023

kdy1 commented Jun 3, 2023

devgony Jun 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunrabbit123 Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

changeset-bot bot commented Dec 29, 2023 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdy1 left a comment

Choose a reason for hiding this comment

CLAassistant commented Dec 18, 2022 •

edited

Loading

devgony Dec 18, 2022 •

edited

Loading

kdy1 Dec 18, 2022 •

edited

Loading

devgony Jun 3, 2023 •

edited

Loading

sunrabbit123 Jul 10, 2023 •

edited

Loading

changeset-bot bot commented Dec 29, 2023 •

edited

Loading