hhvm
hhvm copied to clipboard
facebook
Parse non-fixme /* comments not preceded by newlines
This fixes an issue where the contents of /* ... */ comments are swallowed when they appear on the same line as the previous token.
function foo(string $a): void {
// this currently works
echo(
/* HH_NOT_A_FIXME[4110] */ $a as nonnull);
// this comment is hidden in parser output
echo(/* HH_NOT_A_FIXME[4110] */ $a as nonnull);
}
Edit: this PR also fixes a test that had incorrect expectations:
Given a type alias with comments
type foo = shape(
'a' => bool, /*
some comment
*/ 'b' => string,
);
the comment is currently treated as belonging to 'a' => bool, not 'b' => string, despite the comma separating the two.
Note: this PR does not affect behaviour for HH_FIXME.
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@alexeyt has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
Can you tell me more about the hidden-comment behavior you were trying to fix? What were you doing that resulted in the lost comment?
If I use hh_parse --full-fidelity-json on a file containing your code snippet I see "// this comment is hidden in parser output" as part of the leading trivia of the echo token.
@viratyosin yes! But you don't see the second HH_NOT_A_FIXME[4110] in the non-full-fidelity parse tree, because it's attached to the trailing trivia of the open bracket (as opposed to the leading trivia of the as expression). This causes it to get swallowed by the conversion to the simplified AST I use for Hakana.
@muglug I don't think this solves your problem in the general case. You've changed some trailing trivia to be leading trivia, but AFAICS you'll just end up with cases where you can't see the comment associated with a closing bracket rather than an opening one.
Have you considered using HH_FIXME itself instead? We could easily reserve some number prefix (say 8XXXX) for your use case, and then you'd get this for free.
you'll just end up with cases where you can't see the comment associated with a closing bracket rather than an opening one.
I don't see how that could happen in practice — I never see FIXMEs come after the thing they're intending to fix like this
function foo(string $a): void {
echo($a as nonnull /* FIX_PREVIOUS_ELEMENT[4110] */);
// or
echo(
$a as nonnull /* FIX_NEXT_AFTER_COMMA[4110] */,
$b as nonnull,
);
}
Instead those FIXMEs always come directly preceding the element they're intended to cover.
As to the HH_FIXME[8...] proposal, that's very kind! But I don't think it's a great solution for us — I prefer to use explicit FIXME names rather than force devs to look up what the code means in documentation.
Stuff like
/* HAKANA_FIXME[UnusedFunction] */
is really straightforward, and I prefer it to something like
/* HH_FIXME[8017] -- Hakana detected an unused function */