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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 task done
tolmasky opened this issue Jan 18, 2022 · 1 comment
Open
1 task done

Comments

@tolmasky
Copy link
Contributor

tolmasky commented Jan 18, 2022

馃捇

  • 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 #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, Improve errors location tracking聽#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:

if (trailingCommaPos) {
this.raiseTrailingCommaAfterRest(trailingCommaPos);
}

From:

toAssignableList(
exprList: Expression[],
trailingCommaPos?: ?number,
isLHS: boolean,
): $ReadOnlyArray<Pattern> {

Environment

  • Babel version: 7.16.8

Possible solution

No response

Additional context

No response

@babel-bot
Copy link
Collaborator

Hey @tolmasky! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants