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

Outlier case in Excess Property detection in Union (a case that is -more- strict than it should be) #44856

Open
craigphicks opened this issue Jul 1, 2021 · 9 comments · May be fixed by #49004
Labels
Bug A bug in TypeScript Effort: Difficult Good luck.
Milestone

Comments

@craigphicks
Copy link

craigphicks commented Jul 1, 2021

Bug Report

🔎 Search Terms

Excess Property Checks

(1)

From Release Notes 3.5

In TypeScript 3.5, the type-checker at least verifies that all the provided properties belong to some union member and have the appropriate type, meaning that the sample above correctly issues an error. Note that partial overlap is still permitted as long as the property types are valid.

🕗 Version & Regression Information

3.9.7 - 4.3.4 checked in playground to be the same.

⏯ Playground Link

Playground Link

💻 Code

// The expected rule for properties of union type (|) is  
// (1) must contain all required properties of at least one union member
// (2) may contain additional properties that belong to any union member

{
  type A2 = {a: string; b: string};
  type A3 = {a: string; b: boolean; c: boolean};
  type A4 = {a: string; b: boolean; c: number};

  {
    // THIS IS THE UNEXPECTED OUTLIER (compiler error happens unexpectedly)
    const b: A2|A3 = {a: '', b: '', c: true}; // ❌ assignment strictly checked, extra prop from A3 not allowed
  }
  {
    // BEHAVING AS EXPECTED (no compiler error)
    const b: A2|A3|A4 = {a: '', b: '', c: true}; // ✔ assignments allow extra props of other union types
  }
}

🙁 Actual behavior

The line item marked // THIS IS THE UNEXPECTED OUTLIER is rejected as invalid for A2|A3
even though
{a:''",b:""} contains all properties to satisfy A2, and c:true should be allowed by A3.

The oddity of that is accentuated by noting that the same value can be assigned to A2|A3|A4, where A4 differs only from A3 in having property-type c:number instead of c:boolean.

🙂 Expected behavior

The unexpected rejection of property c should be accepted as it is in the counterexample.


TL;DR : About the actual status quo limited type checking introduced with ts 3.5

From Release Notes 3.5

In TypeScript 3.5, the type-checker at least verifies that all the provided properties belong to some union member and have the appropriate type, meaning that the sample above correctly issues an error. Note that partial overlap is still permitted as long as the property types are valid.

That description does not actually match the observed status quo. For example, that rule would imply that an empty object could always be assigned to a union of object with required properties, and that is obviously false. So surely what the author meant was

... the type-checker at least verifies that all the required properties of some union member are provided ...

i.e., the rule for a set of properties to satisfy a union type (|) is

  • (1) must contain all required properties of at least one union member
  • (2) may contain additional properties that belong to any union member

This actual observed behavior is stronger excess property checking than the original verbiage.

That would still make the outlier behavior in this bug report a bug. It's just worth mentioning while we are on the topic. I have create a separate issue for this #44871.

@MartinJohns
Copy link
Contributor

See #20863.

@craigphicks
Copy link
Author

craigphicks commented Jul 1, 2021

See #20863.

@MartinJohns

That seems to be a post requesting a "Strict Union" - I am familiar with that topic and understand and accept that typescript Union is NOT strict.

The following quote from the rulebook

In TypeScript 3.5, the type-checker at least verifies that all the provided properties belong to some union member and have the appropriate type, meaning that the sample above correctly issues an error. Note that partial overlap is still permitted as long as the property types are valid.

The indicated bug in this report is exhibiting excess property detection stronger than that allowed under the limited excess property detection described by that documentation.

(As described in the lead post section "TL;DR : About the actual status quo limited type checking introduced with ts 3.5", the actual limited excess property detection is somewhat stronger than the above description. However, even then, the bug in this report is still a bug.)

@MartinJohns
Copy link
Contributor

That seems to be a post requesting a "Strict Union"

Not really. It's about excess property checks, and excess property checks can not prevent the passing of additional properties (aka "strict").

@craigphicks
Copy link
Author

craigphicks commented Jul 2, 2021

@MartinJohns -

"It's about excess property checks, and excess property checks can not prevent the passing of additional properties (aka "strict").

Agreed. And the status of #20863 is "needs proposal" and "suggestion", specifically a proposal to change or add to the status quo to enable stronger excess property checking. That is fine - but does not overlap with this post.

In contrast, this bug report accepts the apparent status quo of limited excess-property detection behavior which came with typescript 3.5 (*), and points out an unusual outlier case to that status which appears to be a bug. That outlier case displays unexpected strict type checking, that doesn't follow the status quo of limited type checking.

I am afraid you read the title of this question, saw the phrase "Excess Property detection", and assumed this post must belong as a comment under #20863. I would very much appreciate it if you would look closer. I will modify the title to improve clarity. I appreciate your taking interest and engaging in this report - thank you.

@craigphicks craigphicks changed the title Outlier case in Excess Property detection in Union (|) Outlier case in Excess Property detection in Union (a case that is -more- strict than it should be) Jul 2, 2021
@sandersn sandersn added Bug A bug in TypeScript Effort: Difficult Good luck. labels Jul 2, 2021
@sandersn
Copy link
Member

sandersn commented Jul 2, 2021

Fixing this is quite tricky because the implementation is written cleverly and for speed. Plus, the semantics of excess property checks are so quirky that changing it is very likely to break other code.

@craigphicks
Copy link
Author

Plus, the semantics of excess property checks are so quirky that changing it is very likely to break other code.

The bug is on the side of being too strict. Fixing the bug would only remove compiler errors, not increase them.
Therefore it would not cause old code that compiled to suddenly not be able to compile.

On the other hand, with the bug present, a programmer could make an innocent correct change to their code.
E.g., in the code example, there is correctly no error when A2|A3|A4 is the union, but if A4 is removed from the union, an incorrect error will appear, causing the code to be unable to compile.

... the implementation is written cleverly and for speed.

Can you give a pointer to the code?

@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Jul 7, 2021
@sandersn
Copy link
Member

sandersn commented Jul 8, 2021

In isRelatedTo in checker.ts:

const isPerformingExcessPropertyChecks = !(intersectionState & IntersectionState.Target) && (isObjectLiteralType(source) && getObjectFlags(source) & ObjectFlags.FreshLiteral);
if (isPerformingExcessPropertyChecks) {
    if (hasExcessProperties(source as FreshObjectLiteralType, target, reportErrors)) {
        if (reportErrors) {
            reportRelationError(headMessage, source, originalTarget.aliasSymbol ? originalTarget : target);
        }
        return Ternary.False;
    }
}

The maintenance of intersectionState and ObjectFlags.FreshLiteral is distributed across isRelatedTo. That's the part that's likely to cause problems, since the interactions of various states are confusing.

@craigphicks
Copy link
Author

@sandersn - That is very helpful. It seems you are saying that the problem is in the computation of isPerformingExcessPropertyChecks. Could it not be a problem in the function hasExcessProperties?

Aside: To explain why I appreciate your labeling this issue as a bug, let me quote from a reputable source on runtime type checking in typescript

Strictness of runtime checking

  1. Needs to be at least as strict as compile-time checking (otherwise, we lose the guarantees that the compile-time checking provides)
  2. Can be more strict if desired ...

Let us suppose a runtime type checking of a Union object. The obvious baseline specification would be to mirror. as closely as reasonably possible, the excess property checking of compile time type checking. (Stricter than baseline being optional).

Therefore labeling this issue as a bug has helped establish a reasonable upper bound on the strictness of the definition of union excess property compile type checking. Would the following be an accurate description of an upper bound?

An upper bound on the rules for a set of properties to satisfy a union type (|) is

  1. must contain all required properties of at least one union member
  2. may not contain additional properties that do not belong to any union member

Note that an upper bound is not unique. Cases which violate the the upper bound but do not result in compile time errors are allowed. However, cases which do not violate the upper bound but become compile time errors should be called bugs (e.g., the case in this bug report).

Please note this question is not a repeat of #44871 (now closed) - because that question was not posed in terms of an upper bound. An upper bound definition may be a more practical formal declaration of what is presently an implicit understanding. (Implicit understandings can easily become misunderstandings, that's why a formal declaration is preferred.)

craigphicks added a commit to craigphicks/TypeScript-cph that referenced this issue May 6, 2022
 Changes to be committed:
        modified:   src/compiler/checker.ts
        new file:   tests/baselines/reference/_44856.js
        new file:   tests/baselines/reference/_44856.symbols
        new file:   tests/baselines/reference/_44856.types
        new file:   tests/cases/compiler/_44856.ts

- fix for issue microsoft#44856

- minor modifications to checker.ts

  - Preexisting algorithm:
    - In the case of checking for excess properties being assigned to a union, the logic forks:
      - If there is an index in the union whose type qualifies it as discriminant index, then don't allow excess properties (roughly)
      - otherwise, allow excess properites which satisfy some member of the union

  - Change:
    - Don't consider an index with a boolean type to qualify for being a discriminant index.

- Before:

  - In *src/compiler/checker.ts*, in the function `createUnionOrIntersectionProperty` the preexisting code is

```
if ((isLiteralType(type) || isPatternLiteralType(type) || type === uniqueLiteralType) {
        checkFlags |= CheckFlags.HasLiteralType;
    }
```
  - `isLiteralType(type)` returns true for a boolean type, and setting `checkFlags |= CheckFlags.HasLiteralType` results in a higher up decision to qualify the corresponding index as a discriminated index.

- After:
```
if ((isLiteralType(type) &&
        (!fromIsDiscriminantProperty || !(type.flags & TypeFlags.Boolean && type.flags & TypeFlags.Union)))
    || isPatternLiteralType(type) || type === uniqueLiteralType) {
    checkFlags |= CheckFlags.HasLiteralType;
}
```

- the `fromIsDiscriminantProperty` is to provide the context in which `createUnionOrIntersectionProperty`.
- That is necessary because `createUnionOrIntersectionProperty` is called in other contexts that require the existing behavior.
  - Here is one such scenario from `objectSpread.ts`
```
function f<T, U>(t: T, u: U) {
    return { ...t, ...u, id: 'id' }
}
let overwriteId: { id: string, a: number, c: number, d: string } =
    f({ a: 1, id: true }, { c: 1, d: 'no' })
```
  - Without introducing the condition `fromIsDiscriminantProperty`, overwriteId would have type never.
@craigphicks
Copy link
Author

@sandersn
@RyanCavanaugh
Pull with fix - #49004

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Effort: Difficult Good luck.
Projects
None yet
4 participants