Skip to content

Show both narrowed type and declared type #45870

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
5 tasks done
bmeck opened this issue Sep 14, 2021 · 9 comments
Open
5 tasks done

Show both narrowed type and declared type #45870

bmeck opened this issue Sep 14, 2021 · 9 comments
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@bmeck
Copy link

bmeck commented Sep 14, 2021

Suggestion

πŸ” Search Terms

List of keywords you searched for before creating this issue. Write them down here so that others can find this suggestion more easily and help provide feedback.

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

πŸ“ƒ Motivating Example

https://twitter.com/Rich_Harris/status/1437490023763415043

is roughly a confusion on narrowed vs storage type of a given field/binding

// _: any
let _;

// _: any
_ = new Date();

// _: any, but completions from Date
// type doesn't match usability, only assignability
_.getFullYear();

πŸ’» Use Cases

Current UI / typing only shows what can be assigned to a value storage location given a source text location.
Need UI to show what can be used from a value storage location given a source text location.

Much of the time these should be the same but showing something about the narrowing would apparently remove some confusions, for example (bikeshed):

let _;
_ = new Date();

// _: any
_;

// becomes...

// _: any as Date
_;
@MartinJohns
Copy link
Contributor

MartinJohns commented Sep 14, 2021

After assigning a value the type changes:
image

Trying to pass it to an incompatible type will fail as well:
image

It's not any anymore.

@orta
Copy link
Contributor

orta commented Sep 14, 2021

I think this Workbench Repro shows the oddness being discussed pretty well:

declare function getItem(): Promise<description: string}>

async function main(): Promise<undefined | string> {
  let _;
  _= await getItem()
//^?
  _= _.description
//   ^?
  _= parseInt(_)
//^?
  _= isNaN(_) ? undefined : _
//         ^?
  _= _?.toPrecision(2)
  return _
}

@MartinJohns
Copy link
Contributor

Oh, I see, I misunderstood. It's about the tooltip on the assignment side.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Sep 14, 2021

Since accessors can now diverge in their types and have the same issue, I could imagine a notation like

(get) Date
(set) any

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Sep 14, 2021

One technical problem (I believe) is that in

   let x;
   x = new Date()
// ^?

by the point of assignment, control flow analysis hasn't changed the type of x - it's only after the assignment of x that we'd ever be able to show a different type. As a user, I would expect to see Date, but for all intents and purposes, I think you'd still end up with any.

@justingrant
Copy link
Contributor

Really nice improvement! Would solving this issue also fix the narrowed-by-assignment code below? (playground)

function toString(a: unknown) {
  a = String(a);
  return a;
  // expected return type: String
  // actual return type: unknown
}

toString(42).includes('4')
// ^^ Object is of type 'unknown'.(2571)

// explicitly declaring a return type doesn't work either
function toString2(a: unknown): string {
  a = String(a);
  return a;
  // ^^ Type 'unknown' is not assignable to type 'string'.(2322)
}

Or is this a different problem? If "different problem" then is there a better solution than declaring new variables (which has a runtime footprint that we'd prefer to avoid) or liberally using type assertions downstream from a type-narrowing mutation (which is verbose and bug-prone)?

I'm asking because we're midway through porting the Temporal polyfill from JS to TS. We're just now getting to the part where we add type annotations to function return types and function parameters. But a lot of the old JS polyfill code mutates let variables and (especially) function parameters like in the trivial example above.

@MartinJohns
Copy link
Contributor

@justingrant I don't see how a local variable has any relevant runtime footprint, but you can use a Type Assertion here:

return a as string;

That an unknown variable does not narrow on assignment is intentional.

@justingrant
Copy link
Contributor

justingrant commented Oct 2, 2021

That an unknown variable does not narrow on assignment is intentional.

My naive expectation would be that because if (typeof x === 'string') and if (x) both narrow x, then x = 42 would also narrow it until x is reassigned again.

Is there a good issue or PR to read about the rationale for why one pattern narrows but the other doesn't? TS usually has good reasons for things that seem counter-intuitive, and I assume the same is true here too.

I don't see how a local variable has any relevant runtime footprint

Probably no significant perf footprint (and avoiding reassignment might even be better perf depending on how engines optimize), but:

  • Adding hundreds of new const assignments may impact bundle size somewhat (although hopefully not too much once gzipped)
  • Requires renaming hundreds of parameters and variable names. These aren't simple renaming refactors; instead we must bisect usage between the old name (pre-mutation) and the new name (post-mutation). For one function it's not a big deal, but doing it hundreds of times is both a big time sink as well as introducing risk of getting one of them wrong.
  • Introduces runtime divergence from the original JS source, which is painful because every time we make runtime-visible changes to one polyfill, we prefer to port the change over to the other one so we don't build up "porting debt" that will make it too hard/risky to port larger changes later. Having different variable names across codebases will make this porting harder and bug-prone.
  • It introduces two in-scope variables where there was one before, which introduces additional possible buggy paths later where post-mutation code refers to the "old" variable after the new one has been assigned. (Same reasoning for why the pipeline proposal is preferred vs. creating new temporaries.)

you can use a Type Assertion here:

return a as string;

Yep, adding hundreds of type assertions is the runtime-invisible way to work around this problem and avoids the problems in the bullet points above. But it's not cost-free either:

  • Makes it harder for TS to find actual type bugs
  • Makes subsequent type refactoring much harder because assertions must be manually updated.
  • Verbose; makes code harder to read.
  • Most functions in our codebase don't just need one type assertion like the trivial example above. A typical pattern is that a function parameter is mutated (e.g. coerced from unknown to a string or Record type) as one of the first statements in the function, and then that now-safe-typed parameter is subsequently used extensively for the rest of the function. Every one of those places would need a type annotation. Here's a typical example of the JS code that I'm porting today, where duration needs to be annotated 10 times.
  dateAdd(date, duration, options = undefined) {
    if (!ES.IsTemporalCalendar(this)) throw new TypeError('invalid receiver');
    date = ES.ToTemporalDate(date);
    duration = ES.ToTemporalDuration(duration);
    options = ES.GetOptionsObject(options);
    const overflow = ES.ToTemporalOverflow(options);
    const { days } = ES.BalanceDuration(
      GetSlot(duration, DAYS),
      GetSlot(duration, HOURS),
      GetSlot(duration, MINUTES),
      GetSlot(duration, SECONDS),
      GetSlot(duration, MILLISECONDS),
      GetSlot(duration, MICROSECONDS),
      GetSlot(duration, NANOSECONDS),
      'day'
    );
    return impl[GetSlot(this, CALENDAR_ID)].dateAdd(
      date,
      GetSlot(duration, YEARS),
      GetSlot(duration, MONTHS),
      GetSlot(duration, WEEKS),
      days,
      overflow,
      this
    );
  }

@Andarist
Copy link
Contributor

Andarist commented Jun 9, 2023

by the point of assignment, control flow analysis hasn't changed the type of x - it's only after the assignment of x that we'd ever be able to show a different type. As a user, I would expect to see Date, but for all intents and purposes, I think you'd still end up with any.

@DanielRosenwasser is there any specific reason why CFA doesn't change the type of x directly at the assignment?

I have a hacky fix for this locally that creates a fake FlowFlags.Assignment mutation (but in the checker whereas usually all of that happens in the binder) to proxy a situation in which I read the"post-assignment" type. I don't particularly like this hack though πŸ˜… It feels that it would be easier to just change the type directly at the assignment but I'm not yet sure what that would change conceptually. Without knowing much about CFA's requirements and inner workings it almost feels like this should be a benign change (one that perhaps would require some code changes here and there but still). I have a feeling that I might be missing something here though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants