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

False positive with OptionalAbstractKeyword #879

Closed
tasomaniac opened this issue Apr 24, 2018 · 9 comments
Closed

False positive with OptionalAbstractKeyword #879

tasomaniac opened this issue Apr 24, 2018 · 9 comments
Milestone

Comments

@tasomaniac
Copy link
Contributor

Expected Behavior

I have a valid code (pasted below) that shouldn't have this warning. There is no other way of doing this one actually.

Current Behavior

OptionalAbstractKeyword is warning given below. The abstract keyword is not even in an interface but in another abstract class that is inside an interface.

image

Steps to Reproduce (for bugs)

The code looks like below. It is a Dagger component.

AndroidInjector.Builder is abstract.
@Component.Builder is required to be put on an abstract class.
fun build(): AppComponent is abstract. It is defined in AndroidInjector.Builder actually. It overrides and modifies the return type.

@Component
interface AppComponent : AndroidInjector<App> {

    fun analytics(): Analytics

    @Component.Builder
    abstract class Builder : AndroidInjector.Builder<App>() {

        abstract override fun build(): AppComponent
    }
}

Context

I don't believe I have an error to fix. But other than that the message shown above does not have much information and does not give any additional information about how to fix the issue.

Your Environment

@schalkms
Copy link
Member

I tried to reproduce your error without success. I used your code from the github link (nice project btw). Are you using the latest version of detekt?
The issue message could be improved I guess. It should tell the user about unnecessary abstract modifiers in an interface.

@arturbosch
Copy link
Member

We had some false positives with that rule, I think they should be fixed in RC6.4 or master.
@schalkms he is using RC6.4.

@schalkms
Copy link
Member

I overlooked it. He is using RC6-3 actually ;)

@tasomaniac
Copy link
Contributor Author

Yes, there is a small blocker preventing me to upgrade at the moment. I searched for old issues but couldn't find anything. I will double check.

@marcelpinto
Copy link

Hi, I am having an issue with this rule with sealed classes.

Unless I am mistaken this is okay to do in sealed classes.

sealed class Error {

        abstract val code: Int

        data class Cancelled(override val code: Int = -1) : Error()
        data class Network(override val code: Int = -1) : Error()
        data class Internal(override val code: Int = 500) : Error()
}

I am using the latest RC6-4. An one important remarkt this class is inside an interface.

@marcelpinto
Copy link

Ok, the issue is because the sealed class is inside an interface. If I move it out, it does not complain.

@arturbosch
Copy link
Member

How does your interface look like? I can't reproduce it with:

		it("test") {
			val code = """
				interface Stuff {
					sealed class Error {
						abstract val code: Int

						data class Cancelled(override val code: Int = -1) : Error()
						data class Network(override val code: Int = -1) : Error()
						data class Internal(override val code: Int = 500) : Error()
					}
				}
			"""
			assertThat(subject.lint(code)).hasSize(0)
		}

@schalkms
Copy link
Member

Do we have any update on this issue?

@arturbosch arturbosch added this to the RC7-2 milestone Jun 1, 2018
@lock
Copy link

lock bot commented Jun 20, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related topics.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants