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

Forward-uses are treated as uses of external classes #185

Open
wchargin opened this issue Feb 18, 2018 · 0 comments
Open

Forward-uses are treated as uses of external classes #185

wchargin opened this issue Feb 18, 2018 · 0 comments

Comments

@wchargin
Copy link
Contributor

In Flow, the following declarations are equivalent:

type Y = number;
type X = {
  y: Y;
}

and

type X = {
  y: Y;
}
type Y = number;

This plugin translates the first one correctly, but in the second one
the attribute y of the PropTypes generated for X refers to Y as if
it were an external class, not a Flow type. In practice, this means that
the attribute is equivalent to y: any. Presumably, this is because the
conversion scans linearly across the file and has not yet encountered
the definition of Y.

This occurs frequently in practice. A structure like

export type CommitData = {
  fileToCommits: {[filename: string]: string[]},
  commits: {[commitHash: string]: Commit},
};
type Commit = {
  author: string,
  stats: {[filename: string]: FileStats},
};
type FileStats = {
  lines: number,
  added: number,
  deleted: number,
};

is perfectly readable, but is turned by this plugin into

var bpfrpt_proptype_CommitData = {
  fileToCommits: PropTypes.objectOf(PropTypes.arrayOf(PropTypes.string.isRequired).isRequired).isRequired,
  commits: PropTypes.objectOf(function () {
    return (typeof Commit === "function" ? PropTypes.instanceOf(Commit).isRequired : PropTypes.any.isRequired).apply(this, arguments);
  }).isRequired
};

which is effectively useless because typeof Commit === "undefined" as
Commit does not exist in the code. To get the right structure, one
must permute the source:

type FileStats = {
  lines: number,
  added: number,
  deleted: number,
};
type Commit = {
  author: string,
  stats: {[filename: string]: FileStats},
};
export type CommitData = {
  fileToCommits: {[filename: string]: string[]},
  commits: {[commitHash: string]: Commit},
};

This is much less readable. The high-level concepts should come first
(why do I care about FileStats if I don’t know that I’m talking about
commits or commit data?). But even discarding claims of readability, the
plugin output is clearly doing the wrong thing: equivalent Flow programs
are being translated different, which shouldn’t be the case.

wchargin added a commit to sourcecred/sourcecred that referenced this issue Feb 18, 2018
Summary:
The resulting types less logically structured (as discussed), but while
Flow considers them equivalent, the linter does not—and, more
importantly, the runtime `PropTypes` are not generated correctly when
forward-uses are in play, due to the following issue:

brigand/babel-plugin-flow-react-proptypes#185

I _think_ this is the lesser of two evils—but am happy to be convinced
otherwise.

Test Plan:
Note that `yarn flow` and `yarn test` are both fine, and `yarn start`
has no lint errors or runtime errors. Note further that when reverting
`insertions → added` and `deletions → deleted`, there are prop-type
errors in both `yarn test` and `yarn start`. (Without the permutation of
the types, this does not occur.)

wchargin-branch: avoid-forward-references
teamdandelion pushed a commit to sourcecred/sourcecred that referenced this issue Feb 18, 2018
Summary:
The resulting types less logically structured (as discussed), but while
Flow considers them equivalent, the linter does not—and, more
importantly, the runtime `PropTypes` are not generated correctly when
forward-uses are in play, due to the following issue:

brigand/babel-plugin-flow-react-proptypes#185

I _think_ this is the lesser of two evils—but am happy to be convinced
otherwise.

Test Plan:
Note that `yarn flow` and `yarn test` are both fine, and `yarn start`
has no lint errors or runtime errors. Note further that when reverting
`insertions → added` and `deletions → deleted`, there are prop-type
errors in both `yarn test` and `yarn start`. (Without the permutation of
the types, this does not occur.)

wchargin-branch: avoid-forward-references
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

No branches or pull requests

1 participant