PHPStan\Rules\DisallowedConstructs\DisallowedLooseComparisonRule is not

[VincentLanglet]

Configuration was introduced for rules in #182 (cc @MartinMystikJonas)
But not for PHPStan\Rules\DisallowedConstructs\DisallowedLooseComparisonRule

Is there a reason ?

When using bleedingEdge (for other phpstan feature) it would be nice to still be able to disable the DisallowedLooseComparisonRule.

Should we create a new config parameter which default to %featureToggles.bleedingEdge% ?

[MartinMystikJonas]

I did not touched it because it was already configurable. But I think it is good idea to add it condigurable and defaulted to bleeding edge. @ondrejmirtes Do you agree? I will make PR

[VincentLanglet]

So far all the PHPStan\Rules\DisallowedConstructs rules were under strictRules.disallowedConstructs

Here I wonder if we will have an issue since if we add it under the same config, it will be a bc break
but if we don't, we will loose consistency.

We might also decide to split the strictRules.disallowedConstructs config.

For instance, DisallowedEmptyRule and DisallowedShortTernaryRule are rules which requires a lot of changes in a legacy code base. The DisallowedLooseComparisonRule too. And if I want to disable one of them, I have to disable such easier rules like DisallowedImplicitArrayCreationRule which is kinda sad.

[MartinMystikJonas]

I would preffer to keep BC and just add strictRules.disallowedLooseComparsion.

You can disable strictRules.disallowedConstructs and then add handpicked rules using:

rules:
  - PHPStan\Rules\DisallowedConstructs\DisallowedImplicitArrayCreationRule

[ondrejmirtes]

I think it's a bit tricky to set up the configuration correctly.

So we want to have a new option to turn on/off DisallowedLooseComparisonRule comfortably. It needs to behave this way:

By default:

• if bleedingEdge is not enabled and allRules is enabled, the rule CANNOT be enabled
• if bleedingEdge is enabled and allRules is enabled, the rule MUST be enabled
• if bleedingEdge is not enabled and allRules is not enabled, the rule CANNOT be enabled
• if bleedingEdge is enabled and allRules is not enabled, the rule CANNOT be enabled

With this new option set to true:

• if bleedingEdge is not enabled and allRules is enabled, the rule MUST be enabled
• if bleedingEdge is enabled and allRules is enabled, the rule MUST be enabled
• if bleedingEdge is not enabled and allRules is not enabled, the rule MUST be enabled
• if bleedingEdge is enabled and allRules is not enabled, the rule MUST be enabled

With this new option set to false:

• if bleedingEdge is not enabled and allRules is enabled, the rule CANNOT be enabled
• if bleedingEdge is enabled and allRules is enabled, the rule CANNOT be enabled
• if bleedingEdge is not enabled and allRules is not enabled, the rule CANNOT be enabled
• if bleedingEdge is enabled and allRules is not enabled, the rule CANNOT be enabled

Does this make sense? Can this be achieved in neon config?

[MartinMystikJonas]

@ondrejmirtes I am not sure it is possible in neon. It would require logical operations in config. I will look at it.

[MartinMystikJonas]

@ondrejmirtes Basically we would need a way to do:
DisallowedLooseComparisonRule = strictRule.disallowLooseComparsion ?? (bleedingEdge && strictRules.allRules)
Correct?

[MartinMystikJonas]

It seems impossible in neon. But I have idea. What about make conditonalTags extension accept not only bool but also some struct with advanved conditions? I will think about some resonable format for that

[ondrejmirtes]

Can you imagine doing it with a conditionalIncludes compiler extension?

[MartinMystikJonas]

What about this?

parameters:
  strictRules:
    allRules: true
    disallowedLooseComparsion: [%strictRules.allRules%, %featureToggles.bleedingEdge%]
    # ...

parametersSchema:
  strictRules: structure([
    allRules: bool(),
    disallowedLooseComparsion: anyOf(bool(), arrayOf(bool()))
    # ...
  ])

conditionalTags:
  PHPStan\Rules\DisallowedConstructs\DisallowedLooseComparisonRule:
    phpstan.rules.rule: %strictRules.disallowedLooseComparsion%

And extends ConditionalTagsExtension so it accepts not only bool but also array of bools that all have to be true for tag to be added.

[ondrejmirtes]

I'd like disallowedLooseComparsion: and(%strictRules.allRules%, %featureToggles.bleedingEdge%) if it's possible...

[MartinMystikJonas]

Not sure of it is possible and it definetly would be way more complicated.

[ondrejmirtes]

My issue with [%strictRules.allRules%, %featureToggles.bleedingEdge%] is that it's not obvious whether it's && or ||...

[MartinMystikJonas]

I know it is not ideal but there is no easy way to do it. Problem is that it needs to be evaluated compile-time and nette/di does not support this. So it would (as far as I know) require writing own NeonAdapter and ParametersExtension. Otherwise everything that is not primitive type is converted to Statement and transformed to PHP code evaluated run-time.

Only other option I can think about is [%strictRules.allRules%, '&&' , %featureToggles.bleedingEdge%] and writing own expression evaluation in ConditionalTagsExtension. It can be done (I implemented similar thing for https://github.com/dermatthes/text-template) but it is lots of work and I do not like this format with "random" strings.

[ondrejmirtes]

We already have custom NeonAdapter to expand relative paths based on the directory where the config file is: https://github.com/phpstan/phpstan-src/blob/1.8.x/src/DependencyInjection/NeonAdapter.php

Do you see a way to adapt that?

[MartinMystikJonas]

I need to look at it. It woudl require to detect and() as special type of construct that is transformed to something else than Statement (and I am not sure it would be accepted by later processing in nette/di) and then in ConditionalTagsExtension write code to evaluate these special objects.