Skip to content

API: incorrect declarations in types.ts #24706

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

Open
ajafff opened this issue Jun 5, 2018 · 5 comments
Open

API: incorrect declarations in types.ts #24706

ajafff opened this issue Jun 5, 2018 · 5 comments
Labels
Breaking Change Would introduce errors in existing code Help Wanted You can do this Infrastructure Issue relates to TypeScript team infrastructure
Milestone

Comments

@ajafff
Copy link
Contributor

ajafff commented Jun 5, 2018

The --strictNullChecks PR changed some declarations in types.ts to avoid frequent assertions in the compiler's code. Unfortunately these changes make it really unsafe for API users because they no longer contain the actually nullable types: #22088 (comment) #22088 (comment)

  • ts.Node#parent is no longer optional (which is definitely wrong for SourceFile)
  • ts.Symbol#declarations and ts.Symbol.valueDeclaration are no longer optional, but can still be undefined
  • ts.Type#symbol is no longer optional, but I guess this comment is still up to date: // Symbol associated with type (if any)

These changes should either be reverted or replaced in the published declaration files.
/cc @Andy-MS @weswigham

@weswigham
Copy link
Member

I agree. I mentioned it on the PR. Now that we've done the bulk of the changes, we should fix one at a time and introduce helpers to help the pain.

@RyanCavanaugh RyanCavanaugh added the Infrastructure Issue relates to TypeScript team infrastructure label Jun 5, 2018
@ajafff
Copy link
Contributor Author

ajafff commented Sep 17, 2018

@RyanCavanaugh Do you plan to fix this in the near future? It's a real PITA for me as I use TypeScript's API a lot.
Is there anything I can do to help? Would you accept PRs to fix the issues listed above (probably one at a time to keep the diff small)?

@mike-north
Copy link

mike-north commented Jan 17, 2019

repro case for ts.Symbol.valueDeclaration being undefined

// src/index.ts
export type NumberDict = { [k: string]: number };
// analyze.js
const ts = require('typescript');

const program = ts.createProgram(['src/index.ts'], {
  target: ts.ScriptTarget.ES2015,
  noEmit: true,
  strict: true
});

const checker = program.getTypeChecker();
// get the file
const [indexFile] = program.getSourceFiles().filter(f => !f.isDeclarationFile);

// file's symbol
const indexFileSymbol = checker.getSymbolAtLocation(indexFile);

// file's exports
const { exports: exportSymbols } = indexFileSymbol;

// first exported symbol from the file
const firstExport = exportSymbols.values().next().value;

console.log(
  'First symbol has type of',
  checker.typeToString(checker.getDeclaredTypeOfSymbol(firstExport)),
  'and it has a valueDeclaration of',
  firstExport.valueDeclaration
);

Result: of node analyze.js

First symbol has type of NumberDict and it has a valueDeclaration of undefined

@bradzacher
Copy link
Contributor

bradzacher commented May 7, 2019

We just ran into this bug in @typescript-eslint. One of our rules assumed the typing was correct, causing a bug for an end user.
As an API consumer, we rely on the types to help us ensure we catch bugs at compile time, and it's counter-intuitive to handle cases that are contrary to the types.

Related issues:
typescript-eslint/typescript-eslint#498
typescript-eslint/typescript-eslint#496

ayazhafiz added a commit to ayazhafiz/kythe that referenced this issue Aug 7, 2019
TypeScript doesn't enforce strict non-null checks inside the compiler,
so we should. Perform a non-null check on symbol value declarations,
which are typed as always existing but don't exist for type-only values
like interfaces.

See microsoft/TypeScript#24706
@ayazhafiz
Copy link
Contributor

@weswigham @RyanCavanaugh will you accept changes fixing this?

@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Feb 21, 2020
@RyanCavanaugh RyanCavanaugh added the Help Wanted You can do this label Feb 21, 2020
devversion added a commit to devversion/angular that referenced this issue Apr 9, 2020
… avoid error if base class has no value declaration

The undecorated-classes-with-decorated-fields migration relies on
the type checker to resolve base classes of individual classes.

It could happen that resolved base classes have no value declaration.
e.g. if they are declared through an interface in the default types.
Currently the migration will throw in such situations because it assumes
that `ts.Symbol#valueDeclaration` is always present. This is not the
case, but we don't get good type-checking here due to a bug in the
TypeScript types. See:
microsoft/TypeScript#24706.

Fixes angular#36522.
ayazhafiz pushed a commit to ayazhafiz/TypeScript that referenced this issue Apr 10, 2020
`compiler/types.ts` uses some non-nullable typings to avoid frequent
assertions in the TS compiler. This causes a loss of type information
and makes using typescript's public API dangerous since an external
consumer may assume that a certain type is defined when it is not.
(See microsoft#24706 for more details).

One such case is `Symbol#valueDeclaration`, which this PR provides a
public API typing fix for. This is implemented by adding an additional
processing step to the LGK compiler that overwrites definitions with the
`@optional` JSDoc tag to include the `?` token in their signature.
Specifically, `/** @optional */ valueDeclaration: Declaration` ->
`valueDeclaration?: Declaration`. The relevant change in the public API
definitions is at lib/typescript.d.ts:2285.

Part of microsoft#24706
atscott pushed a commit to angular/angular that referenced this issue Apr 10, 2020
… avoid error if base class has no value declaration (#36543)

The undecorated-classes-with-decorated-fields migration relies on
the type checker to resolve base classes of individual classes.

It could happen that resolved base classes have no value declaration.
e.g. if they are declared through an interface in the default types.
Currently the migration will throw in such situations because it assumes
that `ts.Symbol#valueDeclaration` is always present. This is not the
case, but we don't get good type-checking here due to a bug in the
TypeScript types. See:
microsoft/TypeScript#24706.

Fixes #36522.

PR Close #36543
atscott pushed a commit to angular/angular that referenced this issue Apr 10, 2020
… avoid error if base class has no value declaration (#36543)

The undecorated-classes-with-decorated-fields migration relies on
the type checker to resolve base classes of individual classes.

It could happen that resolved base classes have no value declaration.
e.g. if they are declared through an interface in the default types.
Currently the migration will throw in such situations because it assumes
that `ts.Symbol#valueDeclaration` is always present. This is not the
case, but we don't get good type-checking here due to a bug in the
TypeScript types. See:
microsoft/TypeScript#24706.

Fixes #36522.

PR Close #36543
bradzacher added a commit to typescript-eslint/typescript-eslint that referenced this issue Apr 28, 2020
Simple lint rule to ban usage of properties that introduce bugs.
microsoft/TypeScript#24706
bradzacher added a commit to typescript-eslint/typescript-eslint that referenced this issue Apr 29, 2020
Simple lint rule to ban usage of properties that introduce bugs.
microsoft/TypeScript#24706
@DanielRosenwasser DanielRosenwasser added the Breaking Change Would introduce errors in existing code label May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code Help Wanted You can do this Infrastructure Issue relates to TypeScript team infrastructure
Projects
None yet
Development

No branches or pull requests

7 participants