Skip to content

Fixed the issue with some longer variadic tuples with any rest being incorrectly assignable to shorter variadic tuples #50218

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 2, 2022
Merged

Fixed the issue with some longer variadic tuples with any rest being incorrectly assignable to shorter variadic tuples #50218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 24 additions & 17 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20275,28 +20275,35 @@ namespace ts {
}
const sourceTypeArguments = getTypeArguments(source);
const targetTypeArguments = getTypeArguments(target);
const startCount = Math.min(isTupleType(source) ? getStartElementCount(source.target, ElementFlags.NonRest) : 0, getStartElementCount(target.target, ElementFlags.NonRest));
const endCount = Math.min(isTupleType(source) ? getEndElementCount(source.target, ElementFlags.NonRest) : 0, targetRestFlag ? getEndElementCount(target.target, ElementFlags.NonRest) : 0);
const targetStartCount = getStartElementCount(target.target, ElementFlags.NonRest);
const targetEndCount = getEndElementCount(target.target, ElementFlags.NonRest);
const targetHasRestElement = target.target.hasRestElement;
let canExcludeDiscriminants = !!excludedProperties;
for (let i = 0; i < targetArity; i++) {
const sourceIndex = i < targetArity - endCount ? i : i + sourceArity - targetArity;
const sourceFlags = isTupleType(source) && (i < startCount || i >= targetArity - endCount) ? source.target.elementFlags[sourceIndex] : ElementFlags.Rest;
const targetFlags = target.target.elementFlags[i];
for (let sourcePosition = 0; sourcePosition < sourceArity; sourcePosition++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The core of the fix is in the iteration "style". Previously we were iterating over targetArity and "slicing" a source in the position of the target rest element.

When we consider the added test case:

declare let tt3: [number, string, ...any[]]
let tt4: [number, ...number[]] = tt3

It means that for the target's ...number[] we were creating a slice from those elements: string, ...any[]. This, in turn, was indexed~/unionized and thus became string | any which is just an equivalent of any. And that was used as a source type for the target, introducing a bug.

I've flipped this and now this loop iterates over sourceArity. This ensures that a specific position in the source is assignable, in "isolation", to the respective target position.

Copy link
Member

Choose a reason for hiding this comment

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

From testing this code with some examples and carefully reading through it, I think it's at least as correct as before (and more, now that it fixes the bug). I still need to review the error messages though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for the review! I know that there are some changes in the error messages and stuff - just want to let you know that I would happily make any required adjustments to this PR, but when it comes to those error messages I need to know what would be the expected results. IIRC the current messages are not incorrect - but perhaps you'd like to have them improved somehow.

const sourceFlags = isTupleType(source) ? source.target.elementFlags[sourcePosition] : ElementFlags.Rest;
const sourcePositionFromEnd = sourceArity - 1 - sourcePosition;

const targetPosition = targetHasRestElement && sourcePosition >= targetStartCount
? targetArity - 1 - Math.min(sourcePositionFromEnd, targetEndCount)
: sourcePosition;

const targetFlags = target.target.elementFlags[targetPosition];

if (targetFlags & ElementFlags.Variadic && !(sourceFlags & ElementFlags.Variadic)) {
if (reportErrors) {
reportError(Diagnostics.Source_provides_no_match_for_variadic_element_at_position_0_in_target, i);
reportError(Diagnostics.Source_provides_no_match_for_variadic_element_at_position_0_in_target, targetPosition);
}
return Ternary.False;
}
if (sourceFlags & ElementFlags.Variadic && !(targetFlags & ElementFlags.Variable)) {
if (reportErrors) {
reportError(Diagnostics.Variadic_element_at_position_0_in_source_does_not_match_element_at_position_1_in_target, sourceIndex, i);
reportError(Diagnostics.Variadic_element_at_position_0_in_source_does_not_match_element_at_position_1_in_target, sourcePosition, targetPosition);
}
return Ternary.False;
}
if (targetFlags & ElementFlags.Required && !(sourceFlags & ElementFlags.Required)) {
if (reportErrors) {
reportError(Diagnostics.Source_provides_no_match_for_required_element_at_position_0_in_target, i);
reportError(Diagnostics.Source_provides_no_match_for_required_element_at_position_0_in_target, targetPosition);
}
return Ternary.False;
}
Expand All @@ -20305,24 +20312,24 @@ namespace ts {
if (sourceFlags & ElementFlags.Variable || targetFlags & ElementFlags.Variable) {
canExcludeDiscriminants = false;
}
if (canExcludeDiscriminants && excludedProperties?.has(("" + i) as __String)) {
if (canExcludeDiscriminants && excludedProperties?.has(("" + sourcePosition) as __String)) {
continue;
}
}
const sourceType = !isTupleType(source) ? sourceTypeArguments[0] :
i < startCount || i >= targetArity - endCount ? removeMissingType(sourceTypeArguments[sourceIndex], !!(sourceFlags & targetFlags & ElementFlags.Optional)) :
getElementTypeOfSliceOfTupleType(source, startCount, endCount) || neverType;
const targetType = targetTypeArguments[i];

const sourceType = removeMissingType(sourceTypeArguments[sourcePosition], !!(sourceFlags & targetFlags & ElementFlags.Optional));
const targetType = targetTypeArguments[targetPosition];

const targetCheckType = sourceFlags & ElementFlags.Variadic && targetFlags & ElementFlags.Rest ? createArrayType(targetType) :
removeMissingType(targetType, !!(targetFlags & ElementFlags.Optional));
const related = isRelatedTo(sourceType, targetCheckType, RecursionFlags.Both, reportErrors, /*headMessage*/ undefined, intersectionState);
if (!related) {
if (reportErrors && (targetArity > 1 || sourceArity > 1)) {
if (i < startCount || i >= targetArity - endCount || sourceArity - startCount - endCount === 1) {
reportIncompatibleError(Diagnostics.Type_at_position_0_in_source_is_not_compatible_with_type_at_position_1_in_target, sourceIndex, i);
if (targetHasRestElement && sourcePosition >= targetStartCount && sourcePositionFromEnd >= targetEndCount && targetStartCount !== sourceArity - targetEndCount - 1) {
reportIncompatibleError(Diagnostics.Type_at_positions_0_through_1_in_source_is_not_compatible_with_type_at_position_2_in_target, targetStartCount, sourceArity - targetEndCount - 1, targetPosition);
}
else {
reportIncompatibleError(Diagnostics.Type_at_positions_0_through_1_in_source_is_not_compatible_with_type_at_position_2_in_target, startCount, sourceArity - endCount - 1, i);
reportIncompatibleError(Diagnostics.Type_at_position_0_in_source_is_not_compatible_with_type_at_position_1_in_target, sourcePosition, targetPosition);
}
}
return Ternary.False;
Expand Down
9 changes: 6 additions & 3 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5691,9 +5691,12 @@ namespace ts {

export interface TupleType extends GenericType {
elementFlags: readonly ElementFlags[];
minLength: number; // Number of required or variadic elements
fixedLength: number; // Number of initial required or optional elements
hasRestElement: boolean; // True if tuple has any rest or variadic elements
/** Number of required or variadic elements */
minLength: number;
/** Number of initial required or optional elements */
fixedLength: number;
/** True if tuple has any rest or variadic elements */
hasRestElement: boolean;
Comment on lines +5694 to +5699
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By moving those to JSDocs we improve slightly the IDE experience as this information gets displayed in the tooltips when hovering over those properties

combinedFlags: ElementFlags;
readonly: boolean;
labeledElementDeclarations?: readonly (NamedTupleMember | ParameterDeclaration)[];
Expand Down
3 changes: 3 additions & 0 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2766,8 +2766,11 @@ declare namespace ts {
}
export interface TupleType extends GenericType {
elementFlags: readonly ElementFlags[];
/** Number of required or variadic elements */
minLength: number;
/** Number of initial required or optional elements */
fixedLength: number;
/** True if tuple has any rest or variadic elements */
hasRestElement: boolean;
combinedFlags: ElementFlags;
readonly: boolean;
Expand Down
3 changes: 3 additions & 0 deletions tests/baselines/reference/api/typescript.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2766,8 +2766,11 @@ declare namespace ts {
}
export interface TupleType extends GenericType {
elementFlags: readonly ElementFlags[];
/** Number of required or variadic elements */
minLength: number;
/** Number of initial required or optional elements */
fixedLength: number;
/** True if tuple has any rest or variadic elements */
hasRestElement: boolean;
combinedFlags: ElementFlags;
readonly: boolean;
Expand Down
12 changes: 4 additions & 8 deletions tests/baselines/reference/restTupleElements1.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,10 @@ tests/cases/conformance/types/tuple/restTupleElements1.ts(33,31): error TS2344:
Type 'string' is not assignable to type 'number'.
tests/cases/conformance/types/tuple/restTupleElements1.ts(34,31): error TS2344: Type '[number, number, string]' does not satisfy the constraint '[number, ...number[]]'.
Type at positions 1 through 2 in source is not compatible with type at position 1 in target.
Type 'string | number' is not assignable to type 'number'.
Type 'string' is not assignable to type 'number'.
Type 'string' is not assignable to type 'number'.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A change like this is correct with the new logic. As mentioned in the other comment - I no longer create unions from "tuple slices". That's why the reported source is just a "single" type here and not a union.

I think that perhaps this error could be somehow improved/rephrased cause it mentions a position span in the source where in fact the type mismatch happens only on a given position (or only on some positions of this span). The mentioned span covers all of the positions in the source that must match a single position of the rest in the target.

Note though that reporting a single type here is not completely new, this could always happen in cases like this:

declare let tt3: [number, string, ...string[]];
let tt4: [number, ...number[]] = tt3; 
// Type '[number, string, ...string[]]' is not assignable to type '[number, ...number[]]'.
//   Type at positions 1 through 2 in source is not compatible with type at position 1 in target.
//     Type 'string' is not assignable to type 'number'.(2322)

I think there is a potential of just dropping this specific error:
https://github.com/microsoft/TypeScript/pull/50218/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R20329
in favor of the existing simpler one:
https://github.com/microsoft/TypeScript/pull/50218/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R20332

I've decided not to introduce this change here to limit the scope of the PR and to minimize the initial diff to make it easier to review it.

tests/cases/conformance/types/tuple/restTupleElements1.ts(35,31): error TS2344: Type '[number, number, number, string]' does not satisfy the constraint '[number, ...number[]]'.
Type at positions 1 through 3 in source is not compatible with type at position 1 in target.
Type 'string | number' is not assignable to type 'number'.
Type 'string' is not assignable to type 'number'.
Type 'string' is not assignable to type 'number'.
tests/cases/conformance/types/tuple/restTupleElements1.ts(59,4): error TS2345: Argument of type '[]' is not assignable to parameter of type '[unknown, ...unknown[]]'.
Source has 0 element(s) but target requires 1.

Expand Down Expand Up @@ -94,14 +92,12 @@ tests/cases/conformance/types/tuple/restTupleElements1.ts(59,4): error TS2345: A
~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2344: Type '[number, number, string]' does not satisfy the constraint '[number, ...number[]]'.
!!! error TS2344: Type at positions 1 through 2 in source is not compatible with type at position 1 in target.
!!! error TS2344: Type 'string | number' is not assignable to type 'number'.
!!! error TS2344: Type 'string' is not assignable to type 'number'.
!!! error TS2344: Type 'string' is not assignable to type 'number'.
Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with that change. The message could be better, because what ultimately fails is the type at position 2 in source, so we could be more specific. But considering we had the same problem before this PR, it seems out of scope for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could improve on this error message here or in a followup PR - I would need a confirmation on how this should be reported though. I guess that in this case, I should drop the "positions X through Y" bit and just report this error for the fixed~ position. IIRC this would mean that TS2344 would no longer be reported anywhere - and it's the primary reason why I've left it like this. I wasn't sure how I should approach removing an existing error code.

assign<[number, ...number[]], [number, number, number, string]>(); // Error
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2344: Type '[number, number, number, string]' does not satisfy the constraint '[number, ...number[]]'.
!!! error TS2344: Type at positions 1 through 3 in source is not compatible with type at position 1 in target.
!!! error TS2344: Type 'string | number' is not assignable to type 'number'.
!!! error TS2344: Type 'string' is not assignable to type 'number'.
!!! error TS2344: Type 'string' is not assignable to type 'number'.
Copy link
Member

Choose a reason for hiding this comment

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

Same here, given what we've had before, I think this is good. It actually might be more clear than the previous message, because before we had type 'string | number' mentioned, but the original code doesn't really have that union type, so that could be confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I also found the union type being reported here kinda unintuitive.


type T20 = [number, string, ...boolean[]];

Expand Down
Loading