Workaround for nonnull operator on indexed accesses #19275

weswigham · 2017-10-17T23:52:02Z

We now force the substitution of an indexed access with its base constraint when getting a type with facts, allowing the resulting type to be filtered. The correct approach would be to apply a lazy subtraction type to the indexed access type, and defer the evaluation until required; however @rbuckton is already working on something which can subsume that purpose.

Fixes #17005

sandersn

I'm having trouble with an example of the case that this hack breaks. Can you explain more? (See below for details)

sandersn · 2017-10-18T15:11:55Z

src/compiler/checker.ts

+                // TODO (weswig): This is a substitute for a lazy negated type to remove the types indicated by the TypeFacts from the (potential) union the IndexedAccess refers to
+                //  - instead of defering the resolution of the access, we apply the facts to the base constraint of the access instead; so this works under most circumstances,
+                //    like when the index refers to an optional member and you want to access that member, but unfortunately if that member's type is supposed to vary with the base,
+                //    then the eager evaluation of this removes that relationship


Can you add a short comment to the test that links the test case to this limitation?

Can you put in an example here?

Can you use fewer words if you have an example?

sandersn · 2017-10-18T15:12:35Z

src/compiler/checker.ts

+                //  - instead of defering the resolution of the access, we apply the facts to the base constraint of the access instead; so this works under most circumstances,
+                //    like when the index refers to an optional member and you want to access that member, but unfortunately if that member's type is supposed to vary with the base,
+                //    then the eager evaluation of this removes that relationship
+                const innerType = getBaseConstraintOfType(type) || emptyObjectType;


Name: constraint or baseConstraint?

weswigham · 2017-10-18T19:52:20Z

I'm having trouble with an example of the case that this hack breaks. Can you explain more? (See below for details)

No so much breaks as does not fully fix, for example in:

interface A {
    params?: { name: string; }
}

class Test<T extends A> {
    attrs: Readonly<T>;

    m() {
        return this.attrs.params!;
    }
}

The return type of m should be NonNull<T["params"]> - in master it is T["params"] (no null/undefined removed), and with this change it is {name: string} (which is no longer associated with T). Previously if a subclass gave a more specific "params" member type, the m method would return that more specific type - but now not so much.

And the replacement @rbuckton was working on that could fill in for the desired NonNull type (and obviate the need for this eager workaround) would be

type NonNull<T> = match (T) {
  undefined: never,
  null: never,
  else: T
}

which is why it'd a good idea to track this and remember to replace it when these get in.

weswigham · 2017-10-18T20:01:53Z

@sandersn Got your comments. The bulk of the TODO comment has been replaced with a reference to this PR and the associated test, and the associated test now has an example showing where the current behavior is undesirable. And innerType has been renamed baseConstraint. 🐱

weswigham · 2017-10-18T22:31:46Z

@mmitche The dotnet CI builds on this PR seem to be using an outdated node modules cache (build is failing because it's being build against @types/node@4 instead of 8) - any idea why?

mmitche · 2017-10-19T15:01:59Z

@weswigham No idea...I don't know much about node.

* Adding test case where opened file included in project is not added to ref count of configured project * Fix the way configured project's reference is managed so that the open file * Do not watch root folders for failed lookup locations and effective type roots Fixes #19170 * LEGO: check in for master to temporary branch. * LEGO: check in for master to temporary branch. * Mark fresh spread objects w/ContainsObjectLiteral * Test excess property checks of spreads of unions. * Add exported members of all project files in the global completion list (#19069) * checker.ts: Remove null check on symbols * tsserverProjectSystem.ts: add two tests * client.ts, completions.ts, types.ts: Add codeActions member to CompletionEntryDetails * protocol.ts, session.ts: Add codeActions member to CompletionEntryDetails protocol * protocol.ts, session.ts, types.ts: add hasAction to CompletionEntry * session.ts, services.ts, types.ts: Add formattingOptions parameter to getCompletionEntryDetails * completions.ts: define SymbolOriginInfo type * completions.ts, services.ts: Add allSourceFiles parameter to getCompletionsAtPosition * completions.ts, services.ts: Plumb allSourceFiles into new function getSymbolsFromOtherSourceFileExports inside getCompletionData * completions.ts: add symbolToOriginInfoMap parameter to getCompletionEntriesFromSymbols and to return value of getCompletionData * utilities.ts: Add getOtherModuleSymbols, getUniqueSymbolIdAsString, getUniqueSymbolId * completions.ts: Set CompletionEntry.hasAction when symbol is found in symbolToOriginInfoMap (meaning there's an import action) * completions.ts: Populate list with possible exports (implement getSymbolsFromOtherSourceFileExports) * completions.ts, services.ts: Plumb host and rulesProvider into getCompletionEntryDetails * completions.ts: Add TODO comment * importFixes.ts: Add types ImportDeclarationMap and ImportCodeFixContext * Move getImportDeclarations into getCodeActionForImport, immediately after the implementation * importFixes.ts: Move createChangeTracker into getCodeActionForImport, immediately after getImportDeclarations * importFixes.ts: Add convertToImportCodeFixContext function and reference it from the getCodeActions lambda * importFixes.ts: Add context: ImportCodeFixContext parameter to getCodeActionForImport, update call sites, destructure it, use compilerOptions in getModuleSpecifierForNewImport * importFixes.ts: Remove moduleSymbol parameter from getImportDeclarations and use the ambient one * importFixes.ts: Use cachedImportDeclarations from context in getCodeActionForImport * importFixes.ts: Move createCodeAction out, immediately above convertToImportCodeFixContext * Move the declaration for lastImportDeclaration out of the getCodeActions lambda into getCodeActionForImport * importFixes.ts: Use symbolToken in getCodeActionForImport * importFixes.ts: Remove useCaseSensitiveFileNames altogether from getCodeActions lambda * importFixes.ts: Remove local getUniqueSymbolId function and add checker parameter to calls to it * importFixes.ts: Move getCodeActionForImport out into an export, immediately below convertToImportCodeFixContext * completions.ts: In getCompletionEntryDetails, if there's symbolOriginInfo, call getCodeActionForImport * importFixes.ts: Create and use importFixContext within getCodeActions lambda * importFixes.ts: Use local newLineCharacter instead of context.newLineCharacter in getCodeActionForImport * importFixes.ts: Use local host instead of context.host in getCodeActionForImport * importFixes.ts: Remove dummy getCanonicalFileName line * Filter symbols after gathering exports instead of before * Lint * Test, fix bugs, refactor * Suggestions from code review * Update api baseline * Fix bug if previousToken is not an Identifier * Replace `startsWith` with `stringContainsCharactersInOrder` * Dont try to run unit tests with rwc tests again (#19240) * In tsserver, indent logged JSON (#19080) * noUnusedLocals: Warn for recursive call to private method (#18920) * Added test for windows style paths watched directories * Handle when directory watcher is invoked on file change Fixes #19206 * Add quickfix and refactoring to install @types packages (#19130) * Add quickfix and refactoring to install @types packages * Move `validatePackageName` to `jsTyping.ts` * Remove combinePaths overloads * Respond to code review * Update api baselines * Use native PromiseConstructor * Return false instead of undefined * Remove getProjectRootPath * Update api * This wasnt required before... (#19262) * Collapse jsdoc annotation refactors to one Previously there were two, and two always fired. * Update baselines * Fix #19257: Ensure a generated signature has a return type (#19264) * Fix #19257: Ensure a generated signature has a return type * Ensure generated properties have types * Use the same context for multiple inferences to the same property access * Respect newLine compiler option in language service output (#19279) * LEGO: check in for master to temporary branch. * LEGO: check in for master to temporary branch. * LEGO: check in for master to temporary branch. * Disambiguate same-named refactors using description (#19267) Disambiguate same-named refactors using actionName * Set the scriptKind from the host configuration if present * Update API baselines * Remove erroneous error for JSDoc object literals appears with checkJS. * Update annotateWithTypeFromJSDoc tests 1. Object literals are single-line now. 2. Index signatures transform to TS index signatures. 3. The refactoring is only available when it could add types. * Add a better test for jsdoc index signatures. The test case shows that the errorenous error no longer appears. * Fix bugs in jsdoc annotation refactor 1. Transform index signatures to TS index signatures. 2. Print object literals on a single line. 3. Only offer the refactor when it could add types. (There must not be a type annotation already, and there must be a JSDoc that applies.) * Move isJSDocIndexSignature to utilities * Add failing testcase where when d.ts file is in program, the files get emitted multiple times with --out setting * Modify api to emit affected files using callback instead of generating in memory output Also marking few apis introduced during watch improvements changes that are suppose to be internal for now * Use get files affected by internally and hence use file paths as input * Simplify emit changed files further Also use source file version as the signature of declaration file instead of computing it from text * Do not cache the semantic diagnostics when compiler options has --out since we would anyways get all fresh diagnostics * make getCurrentDirectory required (#19303) * LEGO: check in for master to temporary branch. * Actually use cached semantic diagnostics * Fix tsc-instrumented 1. Make recursiveCreateDirectory correctly handle relative paths. 2. Remove dependency on Harness 3. Correctly increment iocapture0, iocapture1, ... iocaptureN. 4. Stop double-nesting baseline files. * Fix lint * Fix #19270: ensure output name is a valid locale name (#19308) * Fix #19270: ensure output name is a valid locale name * Use const instead of var * Add comment * Fix typo * Split the concat logic for generatedLCGFile * findAllRefs: Support anonymous default export (#19302) * Fix undefined error using `getEffectiveTypeRoots` (#19300) * Remove extra blank line in logs (#19307) * Use Promise instead of PromiseLike (#19305) * Workaround for nonnull operator on indexed accesses (#19275) * Quick and dirty workaround * Add third case to show current behavior * Rename variable, replace elaboration from comment with links * Remove some unnecessary `undefined` checks in extractSymbol (#19256) * Fix "noStringLiteral" lint errors (#19310) * LEGO: check in for master to temporary branch. * Rename locale outputs * Update LKG

Quick and dirty workaround

c35763c

weswigham requested review from sandersn, rbuckton and mhegazy October 17, 2017 23:52

weswigham changed the title ~~Quick and dirty workaround for nonnull operator on indexed accesses~~ Workaround for nonnull operator on indexed accesses Oct 17, 2017

weswigham force-pushed the get-nonnull-index branch from 68476a3 to c35763c Compare October 18, 2017 00:58

sandersn requested changes Oct 18, 2017

View reviewed changes

weswigham added 2 commits October 18, 2017 12:56

Add third case to show current behavior

4107082

Rename variable, replace elaboration from comment with links

c9f8f4a

sandersn approved these changes Oct 18, 2017

View reviewed changes

weswigham merged commit 8212c96 into microsoft:master Oct 19, 2017

weswigham deleted the get-nonnull-index branch October 19, 2017 00:39

microsoft locked and limited conversation to collaborators Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workaround for nonnull operator on indexed accesses #19275

Workaround for nonnull operator on indexed accesses #19275

weswigham commented Oct 17, 2017

sandersn left a comment

sandersn Oct 18, 2017

sandersn Oct 18, 2017

weswigham commented Oct 18, 2017 •

edited

Loading

weswigham commented Oct 18, 2017 •

edited

Loading

weswigham commented Oct 18, 2017

mmitche commented Oct 19, 2017

Workaround for nonnull operator on indexed accesses #19275

Workaround for nonnull operator on indexed accesses #19275

Conversation

weswigham commented Oct 17, 2017

sandersn left a comment

Choose a reason for hiding this comment

sandersn Oct 18, 2017

Choose a reason for hiding this comment

sandersn Oct 18, 2017

Choose a reason for hiding this comment

weswigham commented Oct 18, 2017 • edited Loading

weswigham commented Oct 18, 2017 • edited Loading

weswigham commented Oct 18, 2017

mmitche commented Oct 19, 2017

weswigham commented Oct 18, 2017 •

edited

Loading

weswigham commented Oct 18, 2017 •

edited

Loading