fix(analyzer): Handle multiple docblocks before a statements #818
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently we're only handling docblocks that are tied to a statement, and only consider a single docblock to be tied to any given statement. This means that having multiple consecutive docblocks to declare variables, for example in a classic PHP-only template file, will have us only considering the last of these docblocks. We can improve this by treating all consecutive docblocks that preceed a given statement, only separated by whitespace or comments, to apply to that statement. This uncovered a bug in the docblock_var_on_non_assignment testcase which actually didn't test what it was supposed to test because the extra docblock was ignored. With the new behaviour, that test actually complains that the docblock is redundant because it doesn't narrow the known type. We can widen the original type and add a call that actually requires the type-narrowing docblock to be processed. We can also adjust the test case for issue carthage-software#801 because we now fully support the original code.
dotdash
force-pushed
the
fix_multi_docblock
branch
from
7a572df to
d70510f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently we're only handling docblocks that are tied to a statement, and only consider a single docblock to be tied to any given statement. This means that having multiple consecutive docblocks to declare variables, for example in a classic PHP-only template file, will have us only considering the last of these docblocks. We can improve this by treating all consecutive docblocks that preceed a given statement, only separated by whitespace or comments, to apply to that statement.
This uncovered a bug in the docblock_var_on_non_assignment testcase which actually didn't test what it was supposed to test because the extra docblock was ignored. With the new behaviour, that test actually complains that the docblock is redundant because it doesn't narrow the known type. We can widen the original type and add a call that actually requires the type-narrowing docblock to be processed.
We can also adjust the test case for issue #801 because we now fully support the original code.
📌 What Does This PR Do?
Properly handles multiple consecutive docblocks.
🔍 Context & Motivation
All but the last of a set of consecutive docblocks are currently ignore. This fixes this to process all docblocks
🛠️ Summary of Changes
📂 Affected Areas
🔗 Related Issues or PRs
Fully resolves #801
📝 Notes for Reviewers
<3