RFC: Fix null-check stuff #103

jens-ox · 2021-12-07T15:12:04Z

Hi all!

As mentioned in #98, there are currently quite a few alerts when enabling strict null checks etc in the tsconfig. If it makes sense I'd really like to help, but I'm not sure what the preferred way of handling undefined stuff is.

This PR fixes all TS issues in plainDate.ts when turning on the all the currently commented-out flags in the tsconfig. In most cases, the only change is to allow methods that are called with possibly undefined values to handle that gracefully. Or do you prefer to throw more often if something is undefined?

If this looks ok to you I can start fixing the remaining TS issues.

ptomato

How exciting to receive a PR from someone new to the project and eager to help, thanks very much!

I gave it a quick look and commented on two things that stood out to me. I can take a closer look again later, but it seems like adjusting the GetIntrinsic thing might change a lot of other stuff.

ptomato · 2021-12-07T17:14:25Z

lib/plaindate.ts

-    const Duration = GetIntrinsic('%Temporal.Duration%');
-    return new Duration(years, months, weeks, days, 0, 0, 0, 0, 0, 0);
+    // const Duration = GetIntrinsic('%Temporal.Duration%')!;
+    return new Temporal.Duration(years, months, weeks, days, 0, 0, 0, 0, 0, 0);


This we can't do, because it would break if user code monkey-patched globalThis.Temporal.Duration. (That's the reason for the GetIntrinsic mechanism appearing all over the place, so that we can continue to use the original values internally even if they are replaced in user code.) Sorry, this should've been documented somewhere more clearly! Probably in intrinsic.ts.

I think the best solution here (and elsewhere in the file) would be to type GetIntrinsic such that TypeScript knows that it returns Temporal.Duration for an argument of %Temporal.Duration%. Unfortunately I don't immediately have an idea of how to do that properly 😄

Could this be avoided by just doing import { Duration } from './duration'? Or am I missing something? 😅
Makes sense otherwise, if importing is not sensible here I'll try to teach TypeScript how GetIntrinsic works 😊

I don't think we can import as it causes circular value dependencies between the different files? If that would work that could be one potential fix.

If we need to stick with GetIntrinsic it should already be typed such that a key of %Temporal.Duration% returns typeof Temporal.Duration, but what probably needs to change is that the return type for GetIntrinsic needs to be NonNullable<typeof INTRINSICS[key]> here.

Circular dependencies should be fine as there's already ecmascript -> calendar -> ecmascript :D
If just importing is an option that might be cleaner, right?

How does TypeScript usually handle circular module dependencies?

ptomato · 2021-12-07T17:16:44Z

lib/slots.ts

@@ -275,9 +275,10 @@ export function HasSlot(container: unknown, ...ids: (keyof Slots)[]): boolean {
  return !!myslots && ids.reduce((all: boolean, id) => all && id in myslots, true);
 }
 export function GetSlot<KeyT extends keyof Slots>(
-  container: Slots[typeof id]['usedBy'],
+  container: Slots[typeof id]['usedBy'] | undefined,


I think it signifies a bug in the polyfill if we end up calling GetSlot(undefined, ...) so I'd be happy if TS would complain at compile time rather than resorting to a runtime error. I wonder if there's any way to do that?

In this case it can be undefined if an incorrect disambiguation is given to BuiltinTimeZoneGetInstantFor, so maybe it would be best to just throw?

Maybe we can tighten the types in BuiltinTimeZoneGetInstantFor to ensure at compile time that no incorrect disambiguation can be given there? Or otherwise throw earlier if that happens?

Seems like there may be a legit issue with DisambiguatePossibleInstants because the spec says there should be an assertion that the disambiguation value is valid but the polyfill lacks this assertion. We could add a line at the end of that method e.g.

throw new Error(`assertion failed: invalid disambiguation value ${disambiguation}`);

12wrigja · 2021-12-07T18:14:45Z

A quick drive-by comment: it is probably easiest to fix these flags by starting at the "root-most" files and working outwards. In this project, that would be in slots.ts and ecmascript.ts. Even small changes here will "ripple" through the rest of the files and might mean other fixes are necessary or obsolete.

jens-ox · 2021-12-07T18:16:41Z

A quick drive-by comment: it is probably easiest to fix these flags by starting at the "root-most" files and working outwards. In this project, that would be in slots.ts and ecmascript.ts. Even small changes here will "ripple" through the rest of the files and might mean other fixes are necessary or obsolete.

Yeah, that makes sense. plaindate just seemed like the easiest to understand for a newcomer like me 😊
Once it's clear how all of this can best be done I'll close this PR and work on a "proper" one.

12wrigja · 2021-12-07T18:19:15Z

On second thought, I think probably the two hardest files to change (but definitely most useful...) would be slots.ts and intrinsicclass.ts, as that is where a good chunk of the "type magic" is needed (places where TS can't easily infer the right typing). I'd suggest ecmascript.ts as third.

…mprovements-poc

jens-ox · 2021-12-07T18:57:30Z

I tried removing GetIntrinsic - this doesn't seem to break anything and also has the nice side-effect to clear up some internal naming inconsistencies.

npm run test runs through fine, npm run test262 has problems resolving __ on my machine...

ptomato · 2021-12-07T19:30:06Z

If GetIntrinsic works, that might indeed be cleaner! But I'd be concerned, does it still work when bundling for a platform that doesn't have ES6 modules? I'd also suggest verifying that it really is robust in the face of monkey-patching.

jens-ox · 2021-12-07T19:52:19Z

does it still work when bundling for a platform that doesn't have ES6 modules?

I haven't looked that deeply into the Rollup config used here, but as long as Rollup is properly configured, yes (and there seems to be a CommonJS export generated here, so we should be good to go).

I'd also suggest verifying that it really is robust in the face of monkey-patching

To be honest I never heard of monkey-patching before, so I might have to do a bit of reading first 🙈
But as far as I can see monkey-patching is not possible here.

justingrant · 2021-12-08T02:44:44Z

.gitignore

@@ -5,3 +5,4 @@ tsc-out/
 .vscode/*
 !.vscode/launch.json
 *.tgz
+.DS_STORE


Needs a newline at the end of this line. I think npm run fix should automatically add it. If it doesn't, then you might want to tweak the npm run fix command line args. 😄

justingrant · 2021-12-08T02:50:31Z

lib/calendar.ts

@@ -117,7 +118,7 @@ export class Calendar implements Temporal.Calendar {
    return ES.ToString(this);
  }
  dateFromFields(
-    fields: Params['dateFromFields'][0],
+    fields?: Params['dateFromFields'][0],


fields should never be undefined.

Also, if undefined were acceptable, then for any public API (like this one) it's intentional to leave it as Params... because that enforces alignment between the implementation and index.d.ts. So let's imagine that undefined were OK for this API, then the right fix would be to change index.d.ts, not here.

justingrant · 2021-12-08T02:53:47Z

lib/calendar.ts

@@ -178,7 +179,7 @@ export class Calendar implements Temporal.Calendar {
    if (!ES.IsTemporalCalendar(this)) throw new TypeError('invalid receiver');
    const date = ES.ToTemporalDate(dateParam);
    const duration = ES.ToTemporalDuration(durationParam);
-    const options = ES.GetOptionsObject(optionsParam);
+    const options = ES.GetOptionsObject<Temporal.ArithmeticOptions>(optionsParam);


Why is this explicit type parameter needed? Can't the type be inferred by TS from the type of options ?

justingrant · 2021-12-08T18:44:51Z

lib/intl.ts

@@ -26,6 +25,7 @@ import {
  PlainTimeParams,
  PlainYearMonthParams
 } from './internaltypes';
+import { PlainDateTime } from './plaindatetime';


Does this relative path need to be './plaindatetime.js' because of TS's relatively recent support for Node.js Experimental Modules? From https://www.typescriptlang.org/docs/handbook/esm-node.html#type-in-packagejson-and-new-extensions:

relative import paths need full extensions (we have to write import "./foo.js" instead of import "./foo")

Same applies to all use of imports like this.

justingrant · 2021-12-08T20:30:43Z

lib/ecmascript.ts

@@ -1069,8 +1077,8 @@ export function ToPartialRecord<B extends AnyTemporalLikeType>(
 }

 export function PrepareTemporalFields<B extends AnyTemporalLikeType>(
-  bag: B,
-  fields: ReadonlyArray<FieldRecord<B>>
+  bag?: B,


This is probably the wrong solution. See tc39/proposal-temporal#1963.

justingrant · 2021-12-08T20:59:39Z

lib/ecmascript.ts

 }

 export function ComparisonResult(value: number) {
  return value < 0 ? -1 : value > 0 ? 1 : (value as 0);
 }

-export function GetOptionsObject<T>(options: T) {
+export function GetOptionsObject<T>(options: T | undefined) {


Could you explain why this change is needed? If undefined is missing from the caller's type, then the fix is probably needed in the caller, not here. By adding undefined here we're probably hiding a type bug in the caller.

justingrant · 2021-12-08T21:02:16Z

lib/intrinsicclass.ts

@@ -134,6 +134,3 @@ export function DefineIntrinsic<KeyT extends IntrinsicDefinitionKeys>(name: KeyT
  if (INTRINSICS[key] !== undefined) throw new Error(`intrinsic ${name} already exists`);
  INTRINSICS[key] = value;
 }
-export function GetIntrinsic<KeyT extends keyof typeof INTRINSICS>(intrinsic: KeyT): typeof INTRINSICS[KeyT] {


Not sure this can be completely removed, even if the imports work, because there are some objects and methods (e.g. from methods, maybe intrinsic prototypes) that still need to be accessed.

lib/plaindate.ts

justingrant · 2021-12-08T21:08:58Z

lib/plaindate.ts

@@ -351,6 +350,7 @@ export class PlainDate implements Temporal.PlainDate {
      calendar
    );
    const instant = ES.BuiltinTimeZoneGetInstantFor(timeZone, dt, 'compatible');
+    if (!instant) throw new TypeError(`Could not create instant for timezone "${timeZone}" and dateTime "${dt}"`);


Can instant actually be undefined here? I don't think it can be. The type problem is upstream. For example, DisambiguatePossibleInstants needs an assertion at the end to prevent undefined from being returned, e.g.

throw new Error(`assertion failed: invalid disambiguation value ${disambiguation}`);

justingrant · 2021-12-08T21:17:38Z

lib/slots.ts

@@ -275,9 +275,10 @@ export function HasSlot(container: unknown, ...ids: (keyof Slots)[]): boolean {
  return !!myslots && ids.reduce((all: boolean, id) => all && id in myslots, true);
 }
 export function GetSlot<KeyT extends keyof Slots>(
-  container: Slots[typeof id]['usedBy'],
+  container: Slots[typeof id]['usedBy'] | undefined,


Seems like there may be a legit issue with DisambiguatePossibleInstants because the spec says there should be an assertion that the disambiguation value is valid but the polyfill lacks this assertion. We could add a line at the end of that method e.g.

throw new Error(`assertion failed: invalid disambiguation value ${disambiguation}`);

justingrant · 2021-12-08T21:27:43Z

I'd also suggest verifying that it really is robust in the face of monkey-patching

To be honest I never heard of monkey-patching before, so I might have to do a bit of reading first 🙈 But as far as I can see monkey-patching is not possible here.

Yep, there are a few possible issues. I'm probably missing some, but here's a few things to look at:

Does TS allow the prototype of TS-created classes to be changed? Currently, the polyfill allows calling setPrototypeOf on Temporal types, and if that's the correct behavior then the TS polyfill can't behave differently.
When we create new instances internally in ecmascript.ts by manually using the prototype object, do we use the intrinsic prototype or the monkeypatchable one?
What about static methods like Calendar.from which AFAIK are used internally? Those need to be still accessible internally even if the user monkeypatches them.

The first two bullet points above are covered here: tc39/proposal-temporal#1965

@jens-ox thanks so much for your work on this PR and in uncovering questions like this!

justingrant

Thanks @jens-ox! Lots of notes are inline. You've already helped us find a few issues which is great. Some general feedback:

We'll need to figure out the monkeypatching thing before we know if we can really get rid of GetIntrinsic or not.
I agree with @12wrigja that it'd be best to start upstream with ecmascript.ts, slots, and intrinsics. In general, before you make a change to a method, consider whether the right change to make is upstream from it.
@12wrigja and I can help! Especially with the upstream parts, there's lots of confusing parts of this polyfill. So feel free to tag us with questions if you get stuck.
In general, you should try really hard to avoid runtime behavior changes (e.g. setting default parameters) to work around type issues. There's usually a way to solve those upstream.
Also, adding nullability to parameters usually is incorrect; the problem is usually upstream.
We'll certainly find problems or questions about the proposal-temporal polyfill and/or spec and/or TS types, etc. Examples that I've already found: Polyfill vs. spec mismatch in PrepareTemporalFields tc39/proposal-temporal#1963, prototype property of Temporal types should not be writable tc39/proposal-temporal#1965, Fix TS types for required CalendarProtocol methods tc39/proposal-temporal#1964. So this PR has already been helpful!

jens-ox · 2021-12-08T23:44:12Z

Hi @justingrant! Thanks for your comments. I opened #105 for the GetIntrinsic things. I agree that all the undefined things should be handled upstream (and then probably throw as early as possible).
I'll open a new PR and will start working from the outermost files, avoiding method signature changes.

justingrant · 2022-03-04T06:06:47Z

I think this PR has been replaced by others, so I'm going to close it now. If that's a mistake, feel free to re-open.

fix ts issues in plaindate

52a048a

ptomato reviewed Dec 7, 2021

View reviewed changes

jens-ox added 2 commits December 7, 2021 19:53

remove getIntrinsic

ad911d8

Merge branch 'main' of github.com:jens-ox/temporal-polyfill into ts-i…

5aec4ed

…mprovements-poc

justingrant mentioned this pull request Dec 8, 2021

Polyfill vs. spec mismatch in PrepareTemporalFields tc39/proposal-temporal#1963

Closed

justingrant reviewed Dec 8, 2021

View reviewed changes

justingrant requested changes Dec 8, 2021

View reviewed changes

jens-ox marked this pull request as draft December 8, 2021 23:05

jens-ox mentioned this pull request Dec 8, 2021

GetIntrinsic: remove from PlainDate, add monkeypatching test #105

Open

justingrant closed this Mar 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Fix null-check stuff #103

RFC: Fix null-check stuff #103

jens-ox commented Dec 7, 2021

ptomato left a comment

ptomato Dec 7, 2021

jens-ox Dec 7, 2021

12wrigja Dec 7, 2021

jens-ox Dec 7, 2021

ptomato Dec 9, 2021

ptomato Dec 7, 2021

jens-ox Dec 7, 2021

ptomato Dec 7, 2021

justingrant Dec 8, 2021

12wrigja commented Dec 7, 2021

jens-ox commented Dec 7, 2021

12wrigja commented Dec 7, 2021

jens-ox commented Dec 7, 2021 •

edited

Loading

ptomato commented Dec 7, 2021

jens-ox commented Dec 7, 2021

justingrant Dec 8, 2021

justingrant Dec 8, 2021

justingrant Dec 8, 2021

justingrant Dec 8, 2021

justingrant Dec 8, 2021

justingrant Dec 8, 2021

justingrant Dec 8, 2021

justingrant Dec 8, 2021

justingrant Dec 8, 2021

justingrant commented Dec 8, 2021

justingrant left a comment

jens-ox commented Dec 8, 2021

justingrant commented Mar 4, 2022

RFC: Fix null-check stuff #103

RFC: Fix null-check stuff #103

Conversation

jens-ox commented Dec 7, 2021

ptomato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

12wrigja commented Dec 7, 2021

jens-ox commented Dec 7, 2021

12wrigja commented Dec 7, 2021

jens-ox commented Dec 7, 2021 • edited Loading

ptomato commented Dec 7, 2021

jens-ox commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justingrant commented Dec 8, 2021

justingrant left a comment

Choose a reason for hiding this comment

jens-ox commented Dec 8, 2021

justingrant commented Mar 4, 2022

jens-ox commented Dec 7, 2021 •

edited

Loading