Skip to content
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

Require explicit index checks or assertions #1220

Merged
merged 3 commits into from
Feb 28, 2025

Conversation

scotttrinh
Copy link
Collaborator

@scotttrinh scotttrinh commented Feb 28, 2025

Many places in the code were using unchecked indexed access, which conflicts with many people's TypeScript configurations. We copy the hydrate.ts module directly into the generated code, so we will now check the rest of the generate package with the same strict configuration.

An alternative approach would be to separate out the code we copy into the query builder into a more strict subset and keep the looser config for our own code, but I think stricter is better anyway.

Most places just got the assertion treatment, but in a few places it was straightforward enough to actually do something more safe.

Many places in the code were using unchecked indexed access, which
conflicts with many people's TypeScript configurations. We copy the
hydrate.ts module directly into the generated code, so we will now check
the rest of the generate module with the same strict configuration.

An alternative approach would be to separate out the code we copy into
the query builder into a more strict subset and keep the looser config
for our own code, but I think stricter is better anyway.

Most places just got the assertion treatment, but in a few places it was
straightforward enough to actually do something more safe.
@scotttrinh scotttrinh requested a review from jaclarke February 28, 2025 20:42
@@ -272,7 +272,7 @@ function processLinks(types: JSONType[], modules: string[]): ProcessedSpec {

link_objects.push(lobj);
link["has_link_object"] = true;
objtype["backlinks"][-1]["has_link_object"] = true;
objtype["backlinks"].at(-1)!["has_link_object"] = true;
Copy link
Collaborator Author

@scotttrinh scotttrinh Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vpetrovykh This was the one major change I made across this whole refactor: was this the intention of this code path (update the last backlink in the array)? I think you can use a negative index on an array:

> const a = [];
undefined
> a[-1] = "beep";
'beep'
> a
[ '-1': 'beep' ]

But I doubt that's what you were trying to do here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, your fix is correct, I meant the last element of the array, not the actual index '-1'.

@scotttrinh scotttrinh merged commit d43c393 into master Feb 28, 2025
10 checks passed
@scotttrinh scotttrinh deleted the no-unchecked-indexed-access branch February 28, 2025 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants