improve array index checks

microsoft / vscode-js-profile-visualizer

A custom editor for viewing `.cpuprofile` files in VS Code

MIT License

139 stars 29 forks source link

improve array index checks #160

Closed marcellourbani closed 10 months ago

marcellourbani commented 10 months ago

Fixes #159 I tried to keep changes to a minimum, but there were several instances

connor4312 commented 10 months ago

I appreciate the contribution, but I do not plan to enable indexed array checks in this repository. It just isn't a type of bug I hit often,

in most cases it's provable, such as in the binary search method, that the indexed check will never be out of bounds. In others, out of bounds should never happen if profile input is well-formed, but the far superior way to check this is to do it ahead of time, not at-site whenever the array is accessed (or in the case of this PR, things just fail silently)
there is not always good behavior to take if the array index is out of bounds. For example, if binary search function that you adjusted was flawed, it would just return a nonsense value rather than throwing an error at-site, making any such bugs much harder to find

marcellourbani commented 10 months ago

Thanks for the feedback I agree with most of it, and I was really tempted of throwing an exception in binary search. I agree most errors don't happen with well formed input, and that checking ahead of time would be better than failing in place. Still recovering (when makes sense) or throwing an exception on site would be an improvement ("malformed input" in place of "undefined doesn't have property xxx" ). Not sure I understand the codebase enough to make that call.

connor4312 commented 10 months ago

If you want to add some well-formed checks, these are the model entrypoints:

CPU: https://github.com/microsoft/vscode-js-profile-visualizer/blob/431533b5a226626cb43d61c19d9d65c40df73e3b/packages/vscode-js-profile-core/src/cpu/model.ts#L150
Heap profile: https://github.com/microsoft/vscode-js-profile-visualizer/blob/431533b5a226626cb43d61c19d9d65c40df73e3b/packages/vscode-js-profile-core/src/heap/model.ts#L112
Heap snapshot: already handled by the underlying library

I wish there was a way in the type system to describe that "this type has had a bounds check up to index X and all those indices will be defined." You probably could in some creative way, but then you also need to type that a given numbers is < X which sounds painful...