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

Parse non-fixme /* comments not preceded by newlines #9276

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

muglug
Copy link
Contributor

@muglug muglug commented Oct 27, 2022

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.

@muglug muglug marked this pull request as ready for review October 27, 2022 21:28
@facebook-github-bot
Copy link
Contributor

@muglug has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@muglug has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@alexeyt has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@viratyosin
Copy link
Contributor

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.

@muglug
Copy link
Contributor Author

muglug commented Feb 15, 2023

@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.

@Wilfred
Copy link
Contributor

Wilfred commented Feb 17, 2023

@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.

@muglug
Copy link
Contributor Author

muglug commented Feb 18, 2023

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.

@muglug
Copy link
Contributor Author

muglug commented Feb 18, 2023

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 */

@facebook-github-bot
Copy link
Contributor

@muglug has updated the pull request. You must reimport the pull request before landing.

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

Successfully merging this pull request may close these issues.

4 participants