apache / lucene

Apache Lucene open-source search software
https://lucene.apache.org/
Apache License 2.0
2.45k stars 975 forks source link

Temporarily exclude record ctors and methods from javadoc checks #13329

Closed shubhamvishu closed 1 month ago

shubhamvishu commented 1 month ago

Description

In #13328 we got to know renderSiteJavadoc task fails for javadocs checks are not working as expected with records ctor and methods(synthetic ones). As pointed by @uschindler its hard to find which ones are synthetic and fix it. Meanwhile, we could temporarily disable the javadoc checks for record ctors and methods till the main issue is resolved.

uschindler commented 1 month ago

I am working on a solution, but, e.g., the enum values() are not marked as "mandated" elements. IMHO, they should be "mandated" but not "synthetic":

My idea was to replace the whole method (also for the enums) by this:

  private boolean isSyntheticEnumMethod(Element element) {
    if (elementUtils.getOrigin(element) != Origin.EXPLICIT) {
      System.err.println(element.getSimpleName().toString());
      return true;
    }
    return false;
  }

But the println only lists default constructors, which is bad. So I have no idea how to detect those methods.

uschindler commented 1 month ago

Maybe it works for records, because they are "newer". I will add a fake record for testing.

uschindler commented 1 month ago

With the above code it was able to detect the default constructor of records, but it did not detect the record component accessor methods (same issue like with enums).

uschindler commented 1 month ago

I have a working solution already which works with records. I just want to figure out how to ideally check for missing @param on record components.

I will commit to this PR.

uschindler commented 1 month ago

Here is my code that works:

  /**
   * Return true if the method is synthetic enum (values/valueOf) or record accessor method.
   * According to the doctree documentation, the "included" set never includes synthetic/mandated elements.
   * UweSays: It should not happen but it happens!
   */
  private boolean isSyntheticMethod(Element element) {
    // exclude all not explicitely declared methods
    if (elementUtils.getOrigin(element) != Origin.EXPLICIT) {
      return true;
    }
    // exclude record accessors
    if (element instanceof ExecutableElement ex && elementUtils.recordComponentFor(ex) != null) {
      return true;
    }
    // exclude special enum methods
    String simpleName = element.getSimpleName().toString();
    if (simpleName.equals("values") || simpleName.equals("valueOf")) {
      if (element.getEnclosingElement().getKind() == ElementKind.ENUM) {
        return true;
      }
    }
    return false;
  }
shubhamvishu commented 1 month ago

Nice! This is neat. Thank you Uwe!

I think this takes care of record ctors along with synthetic methods due to ExecutableElement.getEnclosingElement().getEnclosedElements() returning ctor also as enclosed element in Elements#recordComponentFor?

uschindler commented 1 month ago

Nice! This is neat. Thank you Uwe!

I think this takes care of record ctors along with synthetic methods due to ExecutableElement.getEnclosingElement().getEnclosedElements() returning ctor also as enclosed element in Elements#recordComponentFor?

No, the ctors are excluded because of the first if statement. Actually regarding the comment "UweSays: It should not happen but it happens!" the following if statements should be all not needed (because the elements are not explicit - but mandated - it still reports them.

In short:

uschindler commented 1 month ago

I created a new PR #13332 implementing the linter correctly:

uschindler commented 1 month ago

I am closing this in favour of #13332.