AutoService should verify presence of a no-argument public constructor about auto HOT 8 CLOSED

I think that's the TODO here. It does seem like something we could do pretty easly.

from auto.

After looking into this, I find that within Google's source base there are many classes that use @AutoService but don't have public no-arg constructors. Typically it's because they're using a framework that looks for META-INF/services entries, but doesn't use ServiceLocator. For instance the framework might perform injection on the classes it finds. Robolectric does this, for example. So I think requiring a public no-arg constructor probably won't work.

I do think there are some changes we can make:

• Invert the inexplicable default of only checking that the class does implement its @AutoService interface if compiled with -Averify=true.
• Reject @AutoService on interfaces and abstract classes.
• Allow @SuppressWarnings("AutoService") to disable these checks, rather than having to compile with -Averify=false.

from auto.

Typically it's because they're using a framework that looks for META-INF/services entries, but doesn't use ServiceLocator.

Oh, that is an interesting idea.
I'm trying to add @AutoService + ServiceLocator to Apache JMeter instead of the existing "find class files on the classpath": apache/jmeter#5885.
The fact that ServiceLoader instantiates all services it finds does bite me.

By the way, there might be several verify keys like:

• autoServiceVerifyInstanceOf (== the current verify)
• autoServiceVerifyNonAbstract
• autoServiceVerifyDefaultConstructor
• autoServiceVerifyForServiceLoader (== autoServiceVerifyInstanceOf + autoServiceVerifyNonAbstract + autoServiceVerifyDefaultConstructor)

WDYT?

from auto.

I think having several flavours of @SuppressWarnings might be overkill. But perhaps we could have @AutoService(value = MyService.class, forServiceLoader = true) for when you definitely are using ServiceLoader. Then we could check that your class is public and has a public no-arg constructor.

from auto.

I'd rather do forServiceLoader=false (i.e. have it as true by default, allow disabling it for those cases like Robolectric)

from auto.

I do not suggest several @SuppressWarnings. I just thought there might be several properties so the users could select the verifications they need.

For instance, it looks like autoservice verifies nothing by default, and the users have to opt-in by adding -Averify=true.

allow disabling it for those cases like Robolectric

It looks like that the only backward compatible change would be when the new checks are disabled by default since they would break Robolectric-like cases. In my experience, it is always painful if a library upgrade requires changing the nobody knows which configuration.

In that sense, verify becomes a too broad setting, as it would be hard to tell what does it really verify.

So WDYT of keeping verify as it was before for backward compatibility, and adding new properties that either enable individual checks or enable sets of checks (e.g. all checks that are required for ServiceLoader)?

from auto.

I think -A options are usually a poor idea, and I'm not in favour of having more of them. I'd even love to get rid of -Averify, especially since it also has such a global name: who knows what other things besides AutoService are controlled by -Averify? I think code should be understandable by reading it where it is, without having to go and look in some completely different place to figure out what options it might have been compiled with. I also incidentally think javac -parameters is a poor idea too, for the same reason.

If I were designing AutoService from scratch, I would have it do all the ServiceLoader checks by default, with a way to disable some or all of them. However, we have a lot of existing users. There are thousands in Google's internal source base alone, dozens of which would need to have those checks disabled. So I think defaulting to forServiceLoader=true is unfortunately not viable.

It's worth remembering that doing these extra checks means discovering at compile time something that you would otherwise have discovered at run time. If we miss doing the checks, it's just a minor annoyance for the developer. Having to write forServiceLoader=true is also a minor annoyance, and maybe it's better to do nothing beyond the three things I mentioned in my earlier comment.

from auto.

At this stage I think we've done everything we're going to do here.

from auto.