jakartaee / servlet

Jakarta Servlet
https://eclipse.org/ee4j/servlet
Other
261 stars 85 forks source link

Add getAttribute method with generic return type #541

Closed hakan-krgn closed 9 months ago

hakan-krgn commented 1 year ago

This commit adds default getAttribute method with generic return type to ServletContext.

stuartwdouglas commented 1 year ago

If we are going to do this shouldn't we also do the same for ServletRequest? It would also need a changelog entry.

arjantijms commented 1 year ago

Is passing in the class really needed here?

The following should work as well, should it not?

<T> T getAttribute(String name);

Otherwise the call sites would have

Foo foo = (Foo) getAttribute(name);

vs

Foo foo = getAttribute(name, Foo.class);

Which doesn't seem to be much of an improvement tbh.

stuartwdouglas commented 1 year ago

We already have public Object getAttribute(String name), which has the same erasure as <T> T getAttribute(String name).

We can't really modify the existing method without breaking compatibility.

arjantijms commented 1 year ago

We can't really modify the existing method without breaking compatibility.

Just wondering, in what situation would it break?

hakan-krgn commented 1 year ago

Is passing in the class really needed here?

The following should work as well, should it not?

<T> T getAttribute(String name);

Otherwise the call sites would have

Foo foo = (Foo) getAttribute(name);

vs

Foo foo = getAttribute(name, Foo.class);

Which doesn't seem to be much of an improvement tbh.

Yes, that's right. I was going to change return type of public Object getAttribute(String name) method with generic type but I didn't want to change the code structure, that's why I added a default method with generic type.

We already have public Object getAttribute(String name), which has the same erasure as <T> T getAttribute(String name).

We can't really modify the existing method without breaking compatibility.

What kind of compatibility issue are we talking about?

stuartwdouglas commented 1 year ago

So here is an example:


   public static Object getAttribute(String name){
       return null;
   }

//    public static <T> T getAttribute(String name) {
//        return null;
//    }

    static void a(Object a) {
        System.out.println("Object");
    }

    static void a(String a) {
        System.out.println("String");
    }

    static void a(Integer a) {
        System.out.println("int");
    }

    public static void main(String... args) {
        a(getAttribute("ignored"));
    }

If you run the code as it is then you will get the output 'Object'. If you comment out the original version and uncomment the generic version then you will get a compile error because it can't pick between the int and string versions. If you then comment out the Integer version of a() you will get the output 'String'.

arjantijms commented 1 year ago

Totally offtopic but maybe a default for such cases in the JLS would have helped:

    public static <T> T default Object getAttribute(String name) {
        return null;
    }
markt-asf commented 10 months ago
Foo foo = (Foo) getAttribute(name);

vs

Foo foo = getAttribute(name, Foo.class);

Which doesn't seem to be much of an improvement tbh.

Give the above, is this a change we want to pursue? Do we want to look at an alternative such as:

<T> T getTypedAttribute(String name);

or do we just leave the API as-is for now?

stuartwdouglas commented 10 months ago

I am leaning towards leave it as-is, although I am not really sure how big an issue the compatibility problem would be, my example was somewhat contrived and it would probably be very rare for it to actually be a problem.

markt-asf commented 9 months ago

Closing on the basis we will leave this as-is for now. We can always revisit his later if there is interest.