phobos icon indicating copy to clipboard operation
phobos copied to clipboard

disable function attributes dscanner check

Open lucica28 opened this issue 1 year ago • 6 comments

I think this check should be disabled for now, because it does not work as expected. A very brief example:

bool foo() @property { return true; } // [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.
@property bool foo() { return true; } // Should throw a warning but doesn't

I fixed that issue in my fork of D-Scanner, but I can't really move forward until this check is disabled, because one of D-Scanner's tests is to apply it to phobos and have 0 warnings, but I'm getting a lot of warnings from constructions like the second expression mentioned in the example. After I will be done with my project I will probably take the time and fix the warnings D-Scanner throws when applied to phobos, but this is a significant effort, and for now it would be really helpful to disable this check in order to be able to move forward with my project(https://github.com/Dlang-UPB/D-scanner)

lucica28 avatar Dec 11 '22 19:12 lucica28

Thanks for your pull request and interest in making D better, @lucica28! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8648"

dlang-bot avatar Dec 11 '22 19:12 dlang-bot

@lucica28 : Any chance you could take a shot at fixing the issues in Phobos instead of disabling the check ? As @RazvanN7 mentioned, it looks like it applies to more than just @property.

Geod24 avatar Dec 12 '22 12:12 Geod24

@lucica28 : Any chance you could take a shot at fixing the issues in Phobos instead of disabling the check ? As @RazvanN7 mentioned, it looks like it applies to more than just @property.

Well I attempted that, but there are just situation where I'm not 100% sure the check is correct. We can have @property ref functions, where adding const can result in compilation errors when we modify that property. A concrete example: https://github.com/dlang/phobos/blob/master/std/uni/package.d#L6146 here we have a simple stack implementation. The top() method is @property ref, meaning through top we can actually change the state of the object, as would be the case here: https://github.com/dlang/phobos/blob/master/std/regex/internal/parser.d#L283, resulting in a compilation error. Maybe the check doesn't make sense in that particular situation, or maybe I am missing something...?

lucica28 avatar Dec 12 '22 19:12 lucica28

You can partially disable the check in D-Scanner as well

ghost avatar Dec 14 '22 15:12 ghost

Maybe the check doesn't make sense in that particular situation, or maybe I am missing something...?

It should probably be ref inout(T) top () inout.

Geod24 avatar Dec 14 '22 16:12 Geod24

@lucica28, are there other cases (apart from top for which @Geod24 has a good suggestion how to fix) that you're not sure how to handle? I also think that it would better to fix the problematic code in phobos rather than disable the dscanner check.

If you decide to fix the warnings, I suggest you push your changes to this PR (and rename it), to keep the context consolidated for reviewers.

PetarKirov avatar Jan 02 '23 16:01 PetarKirov