Remove unused diagnostic 2346

[Andarist]

I noticed that this code is never executed

[typescript-bot]

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

[fatcerberus]

This is interesting, because googling TS2346 actually turns up results (including at least one issue here) but apparently the error message used to be Supplied parameters do not match any signature of call target.

[fatcerberus]

Also possibly relevant: #17032

[Andarist]

I think that the equivalent diagnostics are just reported by different code paths nowadays. So those errors didn't actually go away.

[fatcerberus]

Yeah, probably fixing #17032 is what made this specific diagnostic unused

[RyanCavanaugh]

ELI5, how do you know this code "never" runs? It appears reachable to me.

[Andarist]

1. I noticed that this diagnostic doesn't appear anywhere in the test suite
2. after removing this branch of code the whole test suite still passed.
3. I replaced the removed code with if (!candidates.length) { throw new Error('here!') }, reran tests and they still passed

[RyanCavanaugh]

Incredibly flattered that you think our test suites are that exhaustive, but it's really not the case 😅. I don't think this is safe to remove unless we ship the assert in the nightly for a couple weeks without getting any hits (which I'm not really keen to do either).

[Andarist]

I've done an additional call hierarchy analysis for this

1. candidates.length is always 0 at the start
2. reorderCandidates fills in candidates based on signatures and after that candidates.length === signatures.length should always be true
3. so the only way to end up with this error is to have signatures.length === 0
4. all but one path leading to this have dedicated branches for their respective signatures.length (like if (constructSignatures.length) in resolveNewExpression etc) and if the list is empty then a dedicated error is added to diagnostic and resolveCall is not called at all
5. the only "unguarded" path is dedicated to super() but I believe that resolveCall can't be called in there either with baseConstructors.length === 0. I both tried different inputs and couldn't make it work and also did an additional analysis here

• this depends on checkSuperExpression's result - if it returns any/error type then we skip the resolveCall (so we can't reach the code that is being removed by this PR)
• that, in turn, depends on baseClassType obtained through getBaseTypes/.resolvedBaseTypes
• those .resolvedBaseTypes can actually be empty but that just means that in such a case we return errorType from checkSuperExpression (since !baseClassType)
• so, in fact, we need a baseClassType to move forward to resolveCall and I think that this implies having at least one valid constructor which also implies that we won't pass an empty signatures list to resolveCall

Incredibly flattered that you think our test suites are that exhaustive, but it's really not the case 😅.

[DanielRosenwasser]

@typescript-bot test this
@typescript-bot test top100
@typescript-bot user test this
@typescript-bot user test tsserver
@typescript-bot test tsserver top100
@typescript-bot run dt

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at 1cacfca. You can monitor the build here.

Update: The results are in!

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite on this PR at 1cacfca. You can monitor the build here.

Update: The results are in!

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 1cacfca. You can monitor the build here.

Update: The results are in!

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at 1cacfca. You can monitor the build here.

Update: The results are in!

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 1cacfca. You can monitor the build here.

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at 1cacfca. You can monitor the build here.

Update: The results are in!

[typescript-bot]

@DanielRosenwasser Here are the results of running the user test suite comparing main and refs/pull/54442/merge:

Everything looks good!

[typescript-bot]

@DanielRosenwasser Here are the results of running the user test suite comparing main and refs/pull/54442/merge:

There were infrastructure failures potentially unrelated to your change:

• 1 instance of "Unknown failure"
• 1 instance of "Package install failed"

Otherwise...

Everything looks good!

[typescript-bot]

@DanielRosenwasser Here are the results of running the top-repos suite comparing main and refs/pull/54442/merge:

Everything looks good!

[typescript-bot]

Hey @DanielRosenwasser, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

[typescript-bot]

@DanielRosenwasser Here are the results of running the top-repos suite comparing main and refs/pull/54442/merge:

Everything looks good!

[RyanCavanaugh]

I'd be OK replacing this with Debug.assert(..., "Revert #54442 and add a testcase with whatever triggered this"); and then doing a full remove in 5.3 😅

[DanielRosenwasser]

@typescript-bot test this
@typescript-bot test top100
@typescript-bot user test this
@typescript-bot user test tsserver
@typescript-bot test tsserver top100
@typescript-bot run dt

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at e79b644. You can monitor the build here.

Update: The results are in!

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at e79b644. You can monitor the build here.

Update: The results are in!

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite on this PR at e79b644. You can monitor the build here.

Update: The results are in!

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at e79b644. You can monitor the build here.

Update: The results are in!

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at e79b644. You can monitor the build here.

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at e79b644. You can monitor the build here.

Update: The results are in!

[typescript-bot]

@DanielRosenwasser Here are the results of running the user test suite comparing main and refs/pull/54442/merge:

Everything looks good!

[typescript-bot]

@DanielRosenwasser Here are the results of running the user test suite comparing main and refs/pull/54442/merge:

There were infrastructure failures potentially unrelated to your change:

• 1 instance of "Unknown failure"
• 1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

rxjs-src

/mnt/ts_downloads/rxjs-src/build.sh

• [NEW] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.collection.d.ts(63,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.iterable.d.ts(162,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.collection.d.ts(63,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.iterable.d.ts(162,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.collection.d.ts(63,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.iterable.d.ts(162,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.collection.d.ts(63,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.iterable.d.ts(162,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.collection.d.ts(63,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.iterable.d.ts(162,11)
  • /home/vsts/work/1/s/typescript-54442/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
• [MISSING] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
  • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)

[typescript-bot]

@DanielRosenwasser Here are the results of running the top-repos suite comparing main and refs/pull/54442/merge:

Everything looks good!

[typescript-bot]

Hey @DanielRosenwasser, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

[typescript-bot]

@DanielRosenwasser Here are the results of running the top-repos suite comparing main and refs/pull/54442/merge:

Everything looks good!

[sandersn]

@RyanCavanaugh is this ready to merge? It looks like it to me.

[rbuckton]

I found something that triggers the debug assertion: #55499

[Andarist]

"Post mortem" of what went wrong with the initial investigation is that this didn't hold true:

so, in fact, we need a baseClassType to move forward to resolveCall and I think that this implies having at least one valid constructor which also implies that we won't pass an empty signatures list to resolveCall

I added further notes on how there might be no valid constructor even in the presence of a baseClassType here:
#55506 (comment)

[RyanCavanaugh]

Two years later, we received another way to hit this (via #61438 (comment))

export class SomeBaseClass { }
export interface SomeInterface { }
export interface MergedClass extends SomeInterface { }
export class MergedClass extends SomeBaseClass<any> {
	public constructor() {
		super();
	}
}

[RyanCavanaugh]

Really a cautionary tale here; I shouldn't have approved this. It's not just the time to author and review PRs, it's the time that users spend tracking these things down -- those people didn't sign up for this work and no one got any tangible benefit from this not-actually-dead code being removed in the interim.