Can you clarify the check that the proposed Analyzer will enforce in the description? The documentation is a bit ambiguous on the syntactic requirements on obj. I think being clearer about what the analyzer will do will help an expert on runtime.SetFinalizer evaluate whether the check makes sense.

runtime.SetFinalizer usage is somewhat uncommon so this may fail the Frequency requirement for cmd/vet:

Vet is run every day by many programmers, often as part of every compilation or
submission. The cost in execution time is considerable, especially in aggregate,
so checks must be likely enough to find real problems that they are worth the
overhead of the added check. A new check that finds only a handful of problems
across all existing programs, even if the problem is significant, is not worth
adding to the suite everyone runs daily.

(Not an obvious no but something to consider.)

What is the failure mode of runtime.SetFinalizer if you pass invalid arguments? If it panics or throws immediately, then I'm not sure a vet check is worth its implementation cost: even trivial test coverage would turn up the error, and finalizers are already subtle enough that no one should be so brazen as to use them without really outstanding test coverage.

How are you supposed to statically determine how a variable was allocated? I don't think this is possible to do nearly precisely enough, even if it were a valuable test to add.

Firstly, thank you all for your nice feedbacks!

@timothy-king Can you clarify the check that the proposed Analyzer will enforce in the description? The documentation is a bit ambiguous on the syntactic requirements on obj. I think being clearer about what the analyzer will do will help an expert on runtime.SetFinalizer evaluate whether the check makes sense.



Ok, I’ll leave a comment about the details soon.

(Basically, I copied the arguments validation logics of runtime.SetFinalizer and transformed them into using AST.)

@timothy-king runtime.SetFinalizer usage is somewhat uncommon so this may fail the Frequency requirement for cmd/vet:


Of course, this check may not meet the frequency requirements as much as unmarshal or structtag analyzers, but given precedents like asmdecl and framepointer, I think it doesn't seem impossible.

@robpike How are you supposed to statically determine how a variable was allocated? I don't think this is possible to do nearly precisely enough, even if it were a valuable test to add.

That’s true. I also think it’s impossible to catch a case where needs runtime information. But, we can still prevent many errors through this analyzer, and since it is not different from what we did in the existing analyzers such as unmarshal and sortslice, I think it’s worth the cost of implementation.

@bcmills

What is the failure mode of runtime.SetFinalizer if you pass invalid arguments? If it panics or throws immediately, then I'm not sure a vet check is worth its implementation cost: even trivial test coverage would turn up the error, and finalizers are already subtle enough that no one should be so brazen as to use them without really outstanding test coverage.

runtime.SetFinalizer throws immediately after checking that obj is not a non-nil pointer, and that finalizer is a function with the correct signature.

@KimMachineGun

But, we can still prevent many errors through this analyzer

Is there an estimate of how many uses of runtime.SetFinalizer in the wild can be statically analyzed by this check?

I agree with @bcmills's implication that this check is not worth adding to vet. Everything that vet can check will be checked at runtime by SetFinalizer. The only way that a vet check could help a real program is if the tests never execute the call to SetFinalizer. That doesn't seem like a significant enough case to worry about.

Sorry for not leaving a comment sooner. I've been very busy these days. 😅

After thinking about this proposal for a few days, I decided to withdraw my proposal.
As many of you've said, this proposal may not seem to be as helpful as I thought before.

Thanks again for your feedbacks.

This proposal has been declined as retracted.
— rsc for the proposal review group