Migrate to TypeScript 5.9 #1799

RichDom2185 · 2025-07-21T10:09:43Z

Description

Supersedes #1666.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Code quality improvements

How to test

Checklist

I have tested this code
I have updated the documentation

coveralls · 2025-08-13T14:33:46Z

Pull Request Test Coverage Report for Build 17011975380

Details

33 of 38 (86.84%) changed or added relevant lines in 15 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage remained the same at 78.56%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/createContext.ts	5	6	83.33%
src/parser/source/typed/typeParser.ts	6	7	85.71%
src/stdlib/pylib.ts	0	1	0.0%
src/utils/stringify.ts	0	2	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/parser/source/typed/typeParser.ts	1	47.84%

Totals
Change from base Build 17011950580:	0.0%
Covered Lines:	8981
Relevant Lines:	11109

💛 - Coveralls

leeyi45 · 2025-08-13T14:41:24Z

Is the stepper officially deprecated (and replaced with the tracer)?

RichDom2185 · 2025-08-13T14:51:13Z

Is the stepper officially deprecated (and replaced with the tracer)?

Is it not just a naming thing? I'm unaware of the developments; this PR tries to be as small as possible and not change any behavior (just trying to update TS)

I do have a subsequent PR that enables verbatimModuleSyntax and migrates the package to ESM, but I don't want to include it here because of scope creep

leeyi45 · 2025-08-13T14:58:24Z

Is the stepper officially deprecated (and replaced with the tracer)?

Is it not just a naming thing? I'm unaware of the developments; this PR tries to be as small as possible and not change any behavior (just trying to update TS)

I do have a subsequent PR that enables verbatimModuleSyntax and migrates the package to ESM, but I don't want to include it here because of scope creep

I ask because I briefly toyed with a similar migration and got tired of fixing the stepper but then I checked and nothing refers to the stepper outside of the stepper folder.

RichDom2185 · 2025-08-13T14:59:42Z

Is the stepper officially deprecated (and replaced with the tracer)?

Is it not just a naming thing? I'm unaware of the developments; this PR tries to be as small as possible and not change any behavior (just trying to update TS)
I do have a subsequent PR that enables verbatimModuleSyntax and migrates the package to ESM, but I don't want to include it here because of scope creep

I ask because I briefly toyed with a similar migration and got tired of fixing the stepper but then I checked and nothing refers to the stepper outside of the stepper folder.

Prof @martin-henz is the stepper gone?

@leeyi45 Either way, I think we can just remove it (if we have to) in a future PR.

leeyi45

In general LGTM, not sure if we want to pursue the path of fewer type assertions and more usage of typeguards and stuff

leeyi45 · 2025-08-13T15:08:21Z

src/transpiler/transpiler.ts

@@ -260,8 +260,8 @@ function transformSomeExpressionsToCheckIfBoolean(program: es.Program, globalIds
    const { line, column } = (node.loc ?? UNKNOWN_LOCATION).start
    const source = node.loc?.source ?? null
    const test = node.type === 'LogicalExpression' ? 'left' : 'test'
-    node[test] = create.callExpression(globalIds.boolOrErr, [
-      node[test],
+    ;(node as any)[test] = create.callExpression(globalIds.boolOrErr, [


I'd rather have this written as

const testNode = node.type === 'LogicalExpression' ? node.left : node.test const callExpr = create.callExpression(... whatever) if (node.type === 'LogicalExpression') { node.left = callExpr } else { node.test = callExpr }

Then we can avoid the type assertion

^^ we should do this, we should aim to reduce the amount of total type assertions/typeguards

Agreed, originally I didn't want this PR to introduce behavioural changes (and leave these cleanup tasks to the future instead) but on second thought it makes sense to do it as part of this PR (since the goal of migrating is for type safety anyway)

Although, having to have these awkward semicolon placements due to the fact the codebase doesn't use semicolons by default bugs me. It may work for plain JS but TS is not the place for no semicolons imo.

What do you think of adding semicolons into the codebase?

Are we expanding the scope of the PR to include general type safety? I can include some changes I was working on for the cse machine

leeyi45 · 2025-08-13T15:10:50Z

src/repl/utils.ts

@@ -23,7 +22,7 @@ export function chapterParser(str: string): Chapter {
  }

  if (foundChapter in Chapter) {


For these, I wrote a typeguard:

export function isKeyOf<T extends Record<string | number | symbol, unknown>>(obj: T, p: string | number | symbol): p is keyof T { return p in obj; }

Then you can just use it like

if (isKeyOf(str, Chapter)) { return Chapter[str] }

^ stylistically I agree and would rather use a typeguard to avoid explicit type assertions

Agreed, thanks, will make the changes

leeyi45 · 2025-08-13T15:16:13Z

src/stepper/converter.ts

@@ -62,7 +62,7 @@ export function nodeToValueWithContext(node: substituterNodes, context: Context)
  return node.type === 'Literal'
    ? node.value
    : util.isBuiltinFunction(node)
-      ? builtin[(node as es.Identifier).name]
+      ? builtin[(node as es.Identifier).name as keyof typeof builtin]


If we change isBuiltInFunction to a proper type guard we won't need the type assertion:

type BuiltinIdentifier = Omit<es.Identifier, 'name'> & { name: keyof typeof builtin } export function isBuiltinFunction(node: SubstituterNodes): node is BuiltinIdentifier { if (!isIdentifier(node) || !isKeyType(builtin, node.name)) return false // ...the other checks }

s-kybound · 2025-08-16T17:09:58Z

Prof @martin-henz is the stepper gone?

The tracer is the new stepper, and should completely replace it. I'm double checking with the ~~stepperv2~~ tracer team, and if all is good I intend to remove the old stepper code completely.

Link: #1811

s-kybound

LGTM with a few nits. I am a bit concerned with the amount of additional type gymnastics we have to do, but that's more to do with the codebase quality than this PR. Additionally, I agree with @leeyi45 and would argue for typeguards > over raw type assertions, but this is more of a stylictic choice.

Should we draft out a style guide for Source Academy (and maybe recommend to write code in ways that avoid the need for playing with the typechecker?)

s-kybound · 2025-08-16T17:56:11Z

src/infiniteLoops/state.ts

@@ -138,7 +138,9 @@ export class State {
    }
    const transitions = this.mixedStack[this.stackPointer].transitions
    for (let i = 0; i < transitions.length; i++) {
-      const transition = transitions[i]
+      // TODO: Something seems to be very wrong with the type definitions here
+      // Shall we just remove the infinite loop detector as per #1516?


Shall we reraise this as an issue? I know that in #1516 it was agreed that the component was useful, but with the problems we are having with it (increased complexity, here we see strange issues with types) we may want to reimplement it (probably only viable as a student project if it's breaking?) or reevaluate it again

Normally, I try to fix type issues as I go (even if it means weird typecasts etc.) but this is beyond me; either it's some super old TS logic that predates my knowledge of the "modern" 4.x and 5.x versions or it's just (ab)use of TS that was somehow never caught.

Without looking at the code and figuring out how this all works I had to resort to just putting an any there.

I suspect the original type definitions may have not been entirely correct, but since TS used to implicitly give an any for these kinds of invalid accesses (and they are accessed in a way consistent and expected of the actual contract/behaviour) then it all just magically seems to work.

I'm ok with removing the feature for now. Re-implementing it will lead to a cleaner implementation.

The project is well-documented in a report and can be revived from there.

I don't think the system will be missed very much. It carries a bit of infrastructure with a server side, sentry support etc that was needed for carrying out the project.

So yea, please go ahead and remove it.

s-kybound · 2025-08-16T18:00:55Z

src/stdlib/pylib.ts

@@ -28,7 +28,8 @@ export function __py_adder(x: Value, y: Value) {
    return x + Number(y)
  }
  if (__is_numeric(x) && __is_numeric(y)) {
-    return x + y
+    // TODO: Shouldn't this be `Number(x) + Number(y)`?


@Fidget-Spinner ^ thoughts? 😅

s-kybound · 2025-08-16T18:07:29Z

src/transpiler/transpiler.ts

@@ -260,8 +260,8 @@ function transformSomeExpressionsToCheckIfBoolean(program: es.Program, globalIds
    const { line, column } = (node.loc ?? UNKNOWN_LOCATION).start
    const source = node.loc?.source ?? null
    const test = node.type === 'LogicalExpression' ? 'left' : 'test'
-    node[test] = create.callExpression(globalIds.boolOrErr, [
-      node[test],
+    ;(node as any)[test] = create.callExpression(globalIds.boolOrErr, [


^^ we should do this, we should aim to reduce the amount of total type assertions/typeguards

s-kybound · 2025-08-16T18:10:09Z

src/repl/utils.ts

@@ -23,7 +22,7 @@ export function chapterParser(str: string): Chapter {
  }

  if (foundChapter in Chapter) {


^ stylistically I agree and would rather use a typeguard to avoid explicit type assertions

…nto ts-58

leeyi45 · 2025-08-17T06:53:32Z

LGTM with a few nits. I am a bit concerned with the amount of additional type gymnastics we have to do, but that's more to do with the codebase quality than this PR. Additionally, I agree with @leeyi45 and would argue for typeguards > over raw type assertions, but this is more of a stylictic choice.

Should we draft out a style guide for Source Academy (and maybe recommend to write code in ways that avoid the need for playing with the typechecker?)

I think a big part of the reason is that a lot of Typescript tricks wouldn't be immediately obvious to someone that hasn't been working with the language for that long so things like Typeguards won't come to mind first.

Like I was staring at the code for the cse machine and there's a ton of type assertions that don't need to be there

RichDom2185 added 3 commits July 21, 2025 18:03

Upgrade TS to 5.8

d2dcdf1

Remove deprecated tsconfig option

7348764

Remove duplicate properties

a392268

RichDom2185 self-assigned this Jul 21, 2025

Merge branch 'master' into ts-58

0b77281

RichDom2185 changed the title ~~Migrate to TypeScript 5.8~~ Migrate to TypeScript 5.9 Aug 13, 2025

RichDom2185 added 8 commits August 13, 2025 21:24

Upgrade TS to v5.9

68e316a

Add types for mathjs

ee7b2e3

Fix some type errors

27bba9d

Update tsconfig

6e252b6

Fix more type errors

3ab92ba

Fix remaining errors

1630263

Update GitHub workflows

63320b0

Fix type error

bad081b

RichDom2185 marked this pull request as ready for review August 13, 2025 14:34

RichDom2185 requested a review from leeyi45 August 13, 2025 14:34

RichDom2185 enabled auto-merge (squash) August 13, 2025 14:34

Update scm-slang to latest

d8d6fa3

leeyi45 reviewed Aug 13, 2025

View reviewed changes

s-kybound requested changes Aug 16, 2025

View reviewed changes

RichDom2185 disabled auto-merge August 16, 2025 18:25

Merge branch 'master' of https://github.com/source-academy/js-slang i…

ab7273d

…nto ts-58

RichDom2185 marked this pull request as draft August 16, 2025 19:29

RichDom2185 mentioned this pull request Aug 16, 2025

Cleanup: Remove infinite loop detector? #1516

Open

Add newline to EOF

0710bcd

		@@ -23,7 +22,7 @@ export function chapterParser(str: string): Chapter {
		}

		if (foundChapter in Chapter) {

Migrate to TypeScript 5.9 #1799

Are you sure you want to change the base?

Migrate to TypeScript 5.9 #1799

Conversation

RichDom2185 commented Jul 21, 2025

Description

Type of change

How to test

Checklist

Uh oh!

coveralls commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 17011975380

Details

💛 - Coveralls

Uh oh!

leeyi45 commented Aug 13, 2025

Uh oh!

RichDom2185 commented Aug 13, 2025

Uh oh!

leeyi45 commented Aug 13, 2025

Uh oh!

RichDom2185 commented Aug 13, 2025

Uh oh!

leeyi45 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-kybound commented Aug 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-kybound left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RichDom2185 Aug 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leeyi45 commented Aug 17, 2025

Uh oh!

Uh oh!

coveralls commented Aug 13, 2025 •

edited

Loading

s-kybound commented Aug 16, 2025 •

edited

Loading

s-kybound left a comment •

edited

Loading

RichDom2185 Aug 16, 2025 •

edited

Loading