PHP 8.5 #[NoDiscard] should probably not be on flock()

[Seldaek]

Description

The following code:

<?php

$fp = fopen("/tmp/lock.txt", "r+");

if (!flock($fp, LOCK_EX)) {
    echo "Couldn't get the lock!";
} else {
    fwrite($fp, "Write something here\n");
    flock($fp, LOCK_UN);
}

fclose($fp);

Resulted in this output:

Warning: The return value of function flock() should either be used or intentionally ignored by casting it as (void), as locking the stream might have failed

But I expected this output instead:

No error

While flock() for LOCK_EX/LOCK_SH operations should definitely be checked for safety, the addition of the #[NoDiscard] attribute there is a bit unfortunate as the LOCK_UN operation doesn't really require checking.

symfony/symfony#60128 is a first symptom of this problem, and I am afraid more will come through. As many projects turn warnings into exceptions this is in practice very close to a BC break.

The new attribute makes sense for cases where not using the return value is definitely going to cause a bug, but for flock it's currently more a strictness check than catching bugs.

PHP Version

8.5

Operating System

No response

[bwoebi]

Yeah, it would need some sort of conditional NoDiscard, but we don't have that. "NoDiscard unless parameter is X" or something.

Like

#[NoDiscard]
function flock($fp, #[AllowDiscard(LOCK_UN)] $mode) {}

But as a short-term measure we probably should remove it now.

[bwoebi]

@TimWolla Do you want to take care about it?

[TimWolla]

Yeah, it would need some sort of conditional NoDiscard, but we don't have that.

And requiring such a functionality is probably an indicator of a questionable API design. flock() mirroring the C API and thus allowing to combine LOCK_UN with LOCK_NB and $would_block would be such a case.

---

But indeed, we did not realize that ignoring the result for LOCK_UN would be reasonable when writing the RFC and no one did during the discussion either. PHP’s test suite did not catch this, since all the calls with LOCK_UN consume the result.

The attribute being on flock() was part of the RFC and voted on, but not implementing that part as an “erratum” should be reasonable without having another vote by simple agreement. I would certainly be open to that.

Alternatively we could perhaps add a new:

function funlock($stream): bool
{
    return \flock($stream, \LOCK_UN);
}

function with a more misuse-resistant API that would also be easily polyfillable.

[nicolas-grekas]

Alternatively we could perhaps add a new: function funlock($stream): bool

That's not really an alternative to the pain imposed on existing code bases, since those would still need to be updated to accommodate for the new warning.

[bwoebi]

I don't think it's questionable API design to have some cases of an API where the return value is not necessarily important.

I also disagree with a dedicated funlock(). We have a way to do it, we don't need two ways. And going through deprecation process etc. is annoying. You'd maybe do it if you're introducing the API the first time...
I'd rather really crave out exemptions where it's not needed or just remove the NoDiscard alltogether.

[TimWolla]

Do you want to take care about it?

Yes, I'll handle that, part of the RFC duty. It would also require a new function in zend_test, since some of the “native function” tests use flock(), thus it's not entirely trivial.

For proper process, I also just sent an email to Internals, pointing towards the issue, allowing folks to object / force a follow-up vote. It should show up somewhere here once externals catches up: https://externals.io/message/126580

[mvorisek]

Maybe revert the whole #[NoDiscard] and leave it for static analysers only?

It not only cannot handle conditional usecases based on params, but in a long term, the result might be not needed as some functions might starting to throw which can depend on the handle, thus the conditionality can depend not only on param, but also on handle settings (like we have for PDO already).

I also have a small question. I know the #[NoDiscard] was introduced mainly for writing safer code. But shouldn't it be applied for all functions like substr where calling the function is pointless? (but the same applied for new, although I know it can call constructor/destructor, IMO static analysers can do a better job here)