[False positive] JSON_THROW_ON_ERROR is not recognized in named parameter call (PHP 8.0)

[niconoe-]

Subject	Details
Plugin	Php Inspections (EA Extended) from 4.0.6.3 (still bugged in 4.0.7.1)
Language level	PHP 8.0, PHP 8.1, PHP 8.2

Current behaviour

When calling json_decode with the flags: names parameter set to JSON_THROW_ON_ERROR, the plugin is rising an error about [EA] Please consider taking advantage of JSON_THROW_ON_ERROR flag for this call options.

Expected behaviour

Expected behaviour is "no error" as I am taking advantage of the Throwable behavior.

Environment details

PhpStorm 2021.1.2
Build #PS-211.7142.44, built on April 30, 2021
Subscription is active until February 22, 2022.
Runtime version: 11.0.10+9-b1341.41 amd64
VM: Dynamic Code Evolution 64-Bit Server VM by JetBrains s.r.o.
Linux 4.15.0-142-generic
GC: G1 Young Generation, G1 Old Generation
Memory: 2048M
Cores: 4
Registry: run.processes.with.pty=TRUE, ide.tooltip.initialDelay=600
Non-Bundled Plugins: au.com.glassechidna.luanalysis (1.2.3), manjaro.mpb (1.5), me.aristotll.python.typing.adder (1.1), mobi.hsz.idea.gitignore (4.1.0), org.plugin.dot.id (1.2), com.kalessil.phpStorm.phpInspectionsEA (4.0.6.3), de.espend.idea.php.annotation (8.0.0), PlantUML integration (5.3.0)
Current Desktop: Unity

Probably some actions have been already taken regarding #1555 or #1592 but those were not about PHP 8.0 and the ability to use a named parameter.

Thanks a lot 🙂

[niconoe-]

I don't speak Java and I never developped a PHPStorm pluggin, so I don't know how I could help in fixing this.

What I think is something can be done in this snippet:

phpinspectionsea/src/main/java/com/kalessil/phpStorm/phpInspectionsEA/inspectors/apiUsage/JsonEncodingApiUsageInspector.java

Lines 78 to 95 in ad57a4a

	if (HARDEN_ERRORS_HANDLING && arguments.length > 0 && PhpLanguageLevel.get(holder.getProject()).atLeast(PhpLanguageLevel.PHP730)) {
	final boolean hasFlag = arguments.length >= 4 && this.hasThrowOnErrorFlag(arguments[3]);
	if (! hasFlag) {
	final String replacement = String.format(
	"%sjson_decode(%s, %s, %s, %s)",
	reference.getImmediateNamespaceName(),
	arguments[0].getText(),
	arguments.length > 1 ? arguments[1].getText() : (HARDEN_DECODING_RESULT_TYPE && DECODE_AS_ARRAY ? "true" : "false"),
	arguments.length > 2 ? arguments[2].getText() : "512",
	arguments.length > 3 ? "JSON_THROW_ON_ERROR | " + arguments[3].getText() : "JSON_THROW_ON_ERROR"
	);
	holder.registerProblem(
	reference,
	MessagesPresentationUtil.prefixWithEa(messageErrorsHandling),
	new HardenErrorsHandlingFix(replacement)
	);
	}
	}

To me, the hasFlag variable is not set correctly, based on what I looked in the code, for several reasons:

1. It does not look at named parameters (and I don't know how the PsiElements are working so I can't propose a PR to help in this)
2. Because of the named parameters, there might be a flag with only 2 parameters, so checking the parameters numbers is no longer sufficient.
3. The method that check the existence of the flag JSON_THROW_ON_ERROR is not taking into account this could be a mask. Therefore, something like ~JSON_THROW_ON_ERROR will not trigger the inspection message, while it should.

To fix this, the hasFlag variable must be set to true when the flag parameter is identified (by name or by position) and analysis of this parameter is conducting to a numerical value that bitwise-matches the according PHP constant value (4194304 for PHP 7.3 and above).

Of course, this applies also to the json_encode method analysis.

Fixing naming recognition and fixing better understanding of the value of this parameter could be done in 2 separated PR.

@kalessil If you or someone you know could be kind enough to explain me how to identify named parameters with the PsiElement object, I could provide a PR that fixes the name recognition even without being a Java developer 🙂

Thank you.

[AllenJB]

This appears to be a duplicate of #1639

[niconoe-]

Thank you @AllenJB I wasn't able to find #1639 when I wrote this issue. It wasn't in my intentions to create a duplicate.

Nevertheless, original issue is more about the fact that the suggestion is incorrect, while it's truely a false positive, implying such suggestion shouldn't be proposed. Also, it is less detailled than this one. Not even mentionning that the "Example Code" section is wrong as the parameter is named flags and not options.

I think it could be helpful to close the original in favor of my issue rather than doing the opposite. Unless you think keeping both is ok and closing both at the same time once a fix will be applied is how the issues are managed in this repository.
That's to the maintainers to decide 😉

[jazithedev]

I "up" for this issue 🙂.

[ezbarrero]

I "UP" also for this issue.

[gabrielmagit]

I also up-vote this issue. I have been experiencing this for a while (since around May or June this year) and it is as described by the OP; getting used to ignoring false-positives isn't good, and this is can be very confusing to less-experienced developers and make them prone to undo correctly-refactored code.

[AlexeyKosov]

And it would be great if it suggested

json_decode($json, flags: JSON_THROW_ON_ERROR);

rather than

json_decode($json, false, 512, JSON_THROW_ON_ERROR);

for PHP 8

[niconoe-]

@AlexeyKosov That would depend on the configuration set on PHPStorm Inspections about "prefer decode into array / prefer decode into object" choice. But yes, if the tool could suggest it without the max_depth that 99.9% of developers don't care, that'd be great! 👍

[CRC-Mismatch]

@niconoe- To me, it seems that either whole implementation around arguments would have to be reworked, or a compromise with conditions made in

phpinspectionsea/src/main/java/com/kalessil/phpStorm/phpInspectionsEA/inspectors/apiUsage/JsonEncodingApiUsageInspector.java

Lines 78 to 95 in ad57a4a

	if (HARDEN_ERRORS_HANDLING && arguments.length > 0 && PhpLanguageLevel.get(holder.getProject()).atLeast(PhpLanguageLevel.PHP730)) {
	final boolean hasFlag = arguments.length >= 4 && this.hasThrowOnErrorFlag(arguments[3]);
	if (! hasFlag) {
	final String replacement = String.format(
	"%sjson_decode(%s, %s, %s, %s)",
	reference.getImmediateNamespaceName(),
	arguments[0].getText(),
	arguments.length > 1 ? arguments[1].getText() : (HARDEN_DECODING_RESULT_TYPE && DECODE_AS_ARRAY ? "true" : "false"),
	arguments.length > 2 ? arguments[2].getText() : "512",
	arguments.length > 3 ? "JSON_THROW_ON_ERROR | " + arguments[3].getText() : "JSON_THROW_ON_ERROR"
	);
	holder.registerProblem(
	reference,
	MessagesPresentationUtil.prefixWithEa(messageErrorsHandling),
	new HardenErrorsHandlingFix(replacement)
	);
	}
	}

Considering it uses positional arguments, and now PHP >8.0 allows for any argument to be in any position, as long as it's named, which means the same goes for all other argument-based validations... 🤷‍♂️ (I guess it's just that this specific case becomes more annoying, since everyone and their neighbor probably skips the max_depth arg anyway)
I'll try to make some time to investigate and try to fix this, but although I'm experienced with Java, I've never even seen how JetBrains plugins work, like yourself...

[niconoe-]

@CRC-Mismatch You're right: every validation that uses arguments without taking their names into account is a possible false-positive. IMO, as [EA] Please consider taking advantage of JSON_THROW_ON_ERROR flag for this call options. is currently being very present because no one uses the max_depth argument, we should try to build an "argument-named" approach to solve this first, then other PRs could be done for other argument-based validations that occurs more rarely for other validations.

[ricardoboss]

@kalessil @CRC-Mismatch @niconoe- Can someone help get my draft PR (#1815) to run? Or test it for me?

[niconoe-]

I only looked at the source code proposal since I’m not working today, and it looks promising! I don’t know how can I test this locally to be honest. I don’t know if there are stuff like unit tests to provide here to secure your proposition, but maybe it could reassure @kalessil 😊

[dcaswel]

This seems to have stalled in October. If I knew Java I would try to help but I would love if we could get this fixed.

[niconoe-]

We're still having this issue opened almost 2 years after its creation, and many duplicates have come during that period (#1873, #1845, #1825, and original #1639).

@ricardoboss Do you know what prevents you to undraft your PR, and could you work on that? Maybe if the PR is ready, @kalessil could merge it?

Thanks!

[ricardoboss]

@niconoe- the PR is still in draft because I am unable to test it. I just assume the code works. Someone with more experience might be able to test it for me and provide feedback. Then it may get out of the draft state.

[VincentLanglet]

The issue is still present.

The PR seems ok ; what about landing it ?

[Simbiat]

Just to confirm it's present in PHPStorm 2024.1, too

PhpStorm 2024.1
Build #PS-241.14494.237, built on March 27, 2024
Licensed to simbiat.ru / Dmitry Kustov
Subscription is active until May 11, 2024.
For non-commercial open source development only.
Runtime version: 17.0.10+8-b1207.12 amd64
VM: OpenJDK 64-Bit Server VM by JetBrains s.r.o.
Windows 11.0
GC: G1 Young Generation, G1 Old Generation
Memory: 5120M
Cores: 16
Registry:
  debugger.new.tool.window.layout=true
  run.processes.with.pty=TRUE
  ide.experimental.ui=true
Non-Bundled Plugins:
  com.jetbrains.space (241.14494.150)
  com.intellij.ml.llm (241.14494.240)
  com.kalessil.phpStorm.phpInspectionsEA (5.0.0.0)