Fix #33436

[jack-williams]

No description provided.

[jack-williams]

All baseline changes are either:

• rewording of but 'T' could be instantiated with a different subtype of constraint '{}' to use constraint unknown.
• Additional error elaboration of 'void' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'unknown'. I guess because void is not related to {}, but it is related to unknown.

[AnyhowStep]

#30637

https://github.com/microsoft/TypeScript/pull/30637/files?file-filters%5B%5D=.ts

The above PR did not modify noConstraintType.

I looked over it quickly but it looks like noConstraintType is some kind of sentinel type?

So, while the constraint is unknown in reality now, the error messages still use the sentinel type and use {} in its output?

And this PR is just updating the sentinel type to match the reality?

Sorry if I sound incoherent =(

[jack-williams]

The error message is a consequence of the fact that noConstraintType is defined as an empty object type, but the error message logic doesn't directly reference noConstraintType, it just looks at the constraint type.

In other words, the error message logic does not look for the sentinel specifically, so it wasn't clear to me whether other code would be the same. I don't know what else uses noConstraintType, or how it uses it, so I wasn't sure if there would be any change in type checking semantics prior to this PR.

The default constraint should be unknown, but it seems there may be a few things that still need tidying up. I'm still not sure whether #32814 is influenced by a legacy implicit constraint.

[weswigham]

Right, noConstraintType is just supposed to be a sentinel (the getConstraintFromTypeParameter and friends functions all check for it to know when to return undefined), but if it is leaking into error messages (or comparisons!?!), changing it is pertinent. The only reason it wasn't changed was because it didn't seem like it needed to be, since "no constraint" isn't actually supposed to be a type.

[weswigham]

OK, so this error changes because reportRelationError accesses target.immediateBaseConstraint directly - it should then treat an undefined target.immediateBaseConstraint the same as a noConstraintType target.immediateBaseConstraint.

[jack-williams]

So reportRelationError should probably use getBaseConstraint instead?

From a brief look I don't think it is leaking into comparisons, at least judging by the direct accesses to type.immediateBaseConstraint in checker.ts.

Given the churn on baselines, should we leave this until the outcome of #33436 is finalised? I wasn't sure if that was still in discussion.

[weswigham]

I think #33436 is pretty directly an artifact of not checking for noConstraintType while using type.immediateBaseConstraint directly. I think we should just fix it by checking for the not base constraint type the same way we check for undefined. We can also make the noConstraintType another unknown instead of an object, just in case, but it shouldn't be required, strictly speaking.

So reportRelationError should probably use getBaseConstraint instead?

Yeah, I think so.

[fatcerberus]

If noConstraintType is just used as a sentinel, does this mean T extends {} will be interpreted as having no constraint?

Then again why do we need a sentinel at all? It seems like if <T> and <T extends unknown> are meant to be equivalent, you could just plug in the unknown upfront and avoid the need for a sentinel value entirely?

[weswigham]

does this mean T extends {} will be interpreted as having no constraint?

No. That's a T with a constraint of {}.

[fatcerberus]

Okay then I’m confused. The way you described it before sounded like “no constraint” was being represented internally as T extends noConstraintType and if noConstraintType is currently {}...

[weswigham]

noConstraintType is always looked for and replaced with undefined by all callers except this error message (which is the bug), along with the circularConstraintType sentinel.

[fatcerberus]

No, I get that - my question is, what makes noConstraintType (as it’s currently defined) actually distinguishable from a legit {}? That’s the part I’m hung up on.

[weswigham]

It's never supposed to be witnessed as a type. So nothing. It could be a clone of the number 4 - it's just a sentinel value. It happens to be a type (with no defined structure) right now, hence the mistake.

[AnyhowStep]

So... Better to leave it as {} or change it to a type with a silly name like "you hecked up" (leave the symptom alone), and fix the bug causing the sentinel value to be "witnessed" (deal with the cause)?

Was there a reason to not just use undefined as the sentinel?

Just curious =x

[jack-williams]

Was there a reason to not just use undefined as the sentinel?

Probably to distinguish between types known to have no constraints, and types with lazy constraints not computed yet.

I updated the other sentinels, just because, but I'm happy to revert those changes. Most of the baselines look fine. I marked a couple I'm not sure about.

[weswigham]

Was there a reason to not just use undefined as the sentinel?

undefined is used for the "has not yet been calculated and cached yet" value. :V

[AnyhowStep]

Why was this closed? o.0

[jack-williams]

I don't think there was an associated issue and I'm just doing my bit to keep the random open PR count down. Can always reopen if there is need!

[AnyhowStep]

Isn't #33436 the associated issue? =x

[jack-williams]

Yes, you're right.

[DanielRosenwasser]

Gotta remove this

[DanielRosenwasser]

The previous error message is wrong ({} isn't "interesting" constraint), but shouldn't the user still see an error message here?

[jack-williams]

Presumably the user still sees Type 'A' is not assignable to type 'B'., but I haven't added the elaboration from the linked proposal of:

'B' could be instantiated with an arbitrary type which could be unrelated to 'A'.

[jack-williams]

@DanielRosenwasser Closing this in favor of #35225 as I messed up some merges.