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

Temporarily exclude record ctors and methods from javadoc checks #13329

Closed

Conversation

shubhamvishu
Copy link
Contributor

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
Copy link
Contributor

uschindler commented Apr 30, 2024

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
Copy link
Contributor

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

@uschindler
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

shubhamvishu commented Apr 30, 2024

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
Copy link
Contributor

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:

  • Elements#recordComponentFor tests every method if it is the accessor of a record component, if it returns something != null, we can exclude it
  • Elements#getOrigin(element) != Origin.EXPLICIT finds all ctors which are not declared

@uschindler
Copy link
Contributor

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

  • exclude default ctors
  • exclude record accessor methods
  • check in level=METHOD that all record components have a documentation (using @param)

@uschindler
Copy link
Contributor

I am closing this in favour of #13332.

@uschindler uschindler closed this Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants