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

Change OptionalUnit to report Unit returned by method implementations in interfaces #3099

Closed
3flex opened this issue Sep 25, 2020 · 5 comments · Fixed by #3108
Closed

Change OptionalUnit to report Unit returned by method implementations in interfaces #3099

3flex opened this issue Sep 25, 2020 · 5 comments · Fixed by #3108

Comments

@3flex
Copy link
Member

3flex commented Sep 25, 2020

Expected Behavior of the rule

This gets flagged by OptionalUnit:

interface Foo {
  fun onMapClicked(point: Point?) = Unit
}

Right now though that use of Unit isn't reported.

Context

This could instead be written as:

interface Foo {
  fun onMapClicked(point: Point?) {}
}

The use of Unit in the first example is therefore optional and should be reported. This means reverting the logic introduced in #1176.

@3flex 3flex changed the title Change OptionalUnit to not ignore default Change OptionalUnit to report Unit returned by method implementations in interfaces Sep 25, 2020
@BraisGabin
Copy link
Member

I think that this is a legit use of Unit. I think that this is better than an empty block. It demostrate more intentionality.

I'm not sure if I prefer that or an empty block with a no-opcomment... But, any way, I don't think that this rule should flag that code.

@schalkms
Copy link
Member

Brais is right. I remember this discussion from another thread. I think we decided to keep it like that, because otherwise the EmptyFunction rule would be triggered.

@3flex
Copy link
Member Author

3flex commented Sep 26, 2020

otherwise the EmptyFunction rule would be triggered

EmptyFunction ignores that case:

    override fun visitNamedFunction(function: KtNamedFunction) {
        super.visitNamedFunction(function)
        if (function.isOpen() || function.isDefaultFunction()) {
            return
        }
    ...
    }

    private fun KtNamedFunction.isDefaultFunction() =
        getParentOfType<KtClass>(true)?.isInterface() == true && hasBody()

So writing this should not flag any rule:

interface Foo {
  fun onMapClicked(point: Point?) {}
}

@arturbosch
Copy link
Member

arturbosch commented Sep 26, 2020

It was an intentional commit back then (7797809) without further context though.
I'm fine with providing a stricter version via a config property.

@schalkms
Copy link
Member

EmptyFunction ignores that case:

It seems like that I missed this. This change came in after the linked PR.
Then I’m fine with changing the behavior back to the old one. I don’t think there’s a need for a config property here. Please go ahead.

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

Successfully merging a pull request may close this issue.

4 participants