Skip to content

Conversation

@PythooonUser
Copy link
Contributor

@PythooonUser PythooonUser commented Aug 7, 2020

Summary

This PR adds support for declare directive lists (backwards compatible). Closes #178.

Adds a valid parse tree for:

declare(strict_types=1, ticks=1);

Additional Questions

  1. I couldn't find any official specs that would indicate that the above is valid (despite PHP not returning any errors)
  2. Other parser (e.g. intelephense) report errors here, i.e. for the encountered ,: Unexpected ','. Expected ')'.
  3. Only three types of declare directives are currently allowed. Should that be highlighted with an error when the user tries to insert something else?
  4. Only literals are considered valid values for the directives. Should that be highlighted with an error when the user tries to insert something else?

@ghost
Copy link

ghost commented Aug 7, 2020

CLA assistant check
All CLA requirements met.

@TysonAndre
Copy link
Contributor

Hi - tolerant-php-parser is used by a lot of projects, and previous PRs have attempted to ensure that those versions would work even after an upgrade from 0.0.x to 0.0.(x+1). - https://github.com/microsoft/tolerant-php-parser/network/dependents?package_id=UGFja2FnZS01NDI3MTMyOTI%3D

For example, if those projects expected $declareStatement->declareDirective to exist (e.g. to check for strict_types), they would encounter notices or misbehave on files that previously worked.

In previous commits, e.g. to support union types, catch class lists, etc, the original datatype of the first list element was preserved, and a new property such as $otherDeclareDirectives was added to the properties (and class constant) that was the list of the remaining values. (and a TODO was added to unify them in the next major release)

» ag '\$other'
src/Node/FunctionReturnType.php
19:    public $otherReturnTypes;

src/Node/Parameter.php
23:    public $otherTypeDeclarations;

src/Node/MissingMemberDeclaration.php
24:    public $otherTypeDeclarations;

src/Node/CatchClause.php
26:    public $otherQualifiedNameList;

src/Node/PropertyDeclaration.php
28:    public $otherTypeDeclarations;

@PythooonUser
Copy link
Contributor Author

Thanks for the clarification! Makes total sense. I will have a look at these examples.

@PythooonUser
Copy link
Contributor Author

Alright, I made a quick fix to address the backwards compatibility of the changes and updated the existing tests accordingly. Let me know what you think :)

Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being slow to look at this. Looks good to me, thank you for the PR!

@roblourens roblourens merged commit 8bb0959 into microsoft:master Aug 23, 2020
@PythooonUser PythooonUser deleted the issue-178 branch August 23, 2020 14:05
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

Successfully merging this pull request may close these issues.

DeclareStatement hardcodes assumption there is a single value (low priority)

3 participants