Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disable function attributes dscanner check #8648

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lucica28
Copy link
Contributor

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)

@dlang-bot
Copy link
Contributor

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"

@@ -46,7 +46,7 @@ logical_precedence_check="enabled"
; Checks for undocumented public declarations
undocumented_declaration_check="enabled"
; Checks for poor placement of function attributes
function_attribute_check="enabled"
function_attribute_check="disabled"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you want to disable this entirely? It seems that this check applies to other attributes (@safe, nogc etc.) as well.

@Geod24
Copy link
Member

Geod24 commented Dec 12, 2022

@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.

@lucica28
Copy link
Contributor Author

@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...?

@ghost
Copy link

ghost commented Dec 14, 2022

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

@Geod24
Copy link
Member

Geod24 commented Dec 14, 2022

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.

@PetarKirov
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants