Clean up return types of `HasAnnotations` implementations

Open
  • Is something wrong?
#1,756 1 comment 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
5/5
Estimated time
Over a week
Newbie friendliness
18/100
Issue type
Refactor
Clarity
Mostly clear
Activity status
Active
Tech stack
java
Domain
tooling

Research direction

Start with the default getAnnotationOfType and getAnnotations in HasAnnotations.java, then compare them with the overrides in JavaCodeUnit.java and the casts in AccessTarget.java. The open question is whether JavaMember and JavaCodeUnit should become self-typed generics, which is a breaking API change that needs maintainer agreement before any code changes. Done means a chosen approach, agreed in the thread, with the redundant overrides and casts removed.

Written by the indexing model from the issue text.

Description

breaking change

Problem

HasAnnotations.java has default implementations for most of its methods, e.g.:

    default JavaAnnotation<? extends SELF> getAnnotationOfType(String typeName) {
        Optional<? extends JavaAnnotation<? extends SELF>> annotation = tryGetAnnotationOfType(typeName);
        if (!annotation.isPresent()) {
            throw new IllegalArgumentException(String.format("%s is not annotated with @%s", getDescription(), typeName));
        }
        return annotation.get();
    }

A lot classes also inherit the implementations of JavaMember, e.g.:

    public Set<? extends JavaAnnotation<? extends JavaMember>> getAnnotations() {
        return ImmutableSet.copyOf(annotations.values());
    }

However, some existing implementations have formally incompatible return types although the behaviour is entirely identical to the default implementation (e.g. JavaCodeUnit.java):

    @Override
    @SuppressWarnings("unchecked") // we know the 'owning' member is this code unit
    public JavaAnnotation<? extends JavaCodeUnit> getAnnotationOfType(String typeName) {
        return (JavaAnnotation<? extends JavaCodeUnit>) super.getAnnotationOfType(typeName);
    }

We should consider cleaning up the types here so we can get rid of redundant overrides and typecasts as much as possible.

However, this is a breaking change since this changes return types of external parameters.

Ideas

I am not yet sure what the best approach is. Some thoughts (feel free to edit this post):

  • Just remove all typecasting overrides
    • Quite easy (regex search for ^\s*return \(.*super, also check instances of @SuppressWarnings("unchecked") to be sure)
  • ❓ Is it a problem that e.g. JavaField only implements HasAnnotations<JavaMember> and not HasAnnotations<JavaField>?
    • If that is the case, JavaMember will also need to be generic (JavaMember<SELF extends JavaMember<SELF>>) and the same for JavaCodeUnit -> JavaCodeUnit<SELF extends JavaCodeUnit<SELF>>
    • This also applies to a bunch of casts in AccessTarget.java, e.g. return (Optional<JavaField>) super.resolveMember();
    • Making JavaMember and JavaCodeUnit self-aware comes with a significant number of code changes, and I am not yet 100 % convinced that it will work out.
Dominant language
Java
Stars
3.9k
Forks
350
Avg merge
3d 2m
Merged PRs (30d)
13

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from TNG/ArchUnit

All issues in TNG/ArchUnit

Similar issues

More Java issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.