babel / babel/babel

[Bug]: Update remaining interfaces that use Position/index pairs to just Position

Open
#14,173 1 comment 0 reactions 1 assignee Claimed by @tolmasky View on GitHub
pkg: parser
Dominant language
TypeScript
Stars
44k
Forks
6k
Avg merge
5d 15h
Merged PRs (30d)
23

Description

### 💻

- [X] Would you like to work on a fix?

### How are you using Babel?

Programmatic API (`babel.transform`, `babel.parse`)

### Input code

N/A

### Configuration file name

_No response_

### Configuration

_No response_

### Current and expected behavior

Ever since https://github.com/babel/babel/pull/14130, functions that take Position/index pairs are now redundant. That PR has already accounted for this in error-related functions, but I'd now like to do the same for the remaining function interfaces to just take in the `Position`. Beyond simplifying the code, there are also two correctness considerations that this would address:

1. First, as these pairs always represent the same "location", it would remove the possibility of future accidental errors where the source index and line/column location get out of sync. With this new setup, the one-to-one mapping between these two is made more concrete and there is no opportunity to have to derive them separately.

2. Perhaps more importantly, there are a number of instances where we use either the `?number` type, the `pos: ?number` optional parameter, or both `pos?: ?number`. These are troublesome because Flow seems to not catch uses of "!pos", which are ambiguous and actually mean "not null and not undefined **and not 0**", where you probably mean "not null/undefined". Once we exclusively use `Position` objects, `?Position` will be unambiguous in the sense that `Position { index: 0 }` can never accidentally be interpreted as a "missing" position. I don't believe we currently have any of these issues, but this is somewhat due to "luck". For example, https://github.com/babel/babel/pull/14130 removed this code which only works because it happens to be that a trailing comma couldn't exist at "pos" 0, but is arguably from the isolated context of the function's perspective, incorrect, and you could certainly imagine other such cases eventually popping up in parts of the code where pos *could* be equal to 0:

https://github.com/babel/babel/blob/a6ca39cebb961255e825f3de8e8f59400df4db01/packages/babel-parser/src/parser/lval.js#L231-L233

From:

https://github.com/babel/babel/blob/a6ca39cebb961255e825f3de8e8f59400df4db01/packages/babel-parser/src/parser/lval.js#L207-L211

### Environment

- Babel version: 7.16.8

### Possible solution

_No response_

### Additional context

_No response_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.