Issue #11166: remove getFileContents from EmptyLineSeparator

[mahfouz72]

Issue #11166 :
refactor EmptyLineSeparatorCheck to use AST-based logic not getFileContents()

Diff Regression config: https://gist.githubusercontent.com/mahfouz72/afd57aa6e51ca161b7927acd959e5211/raw/9384f843297bb2e10c074bcb33ddc46902d6bd52/check.xml

WIP. just wants to get an initial review from CIs and maintainers

[mahfouz72]

dealing with comment tokens in the AST was weird to me. why it is under modifiers of the next subtree. is there a reason to build AST like this?

  int x;
    // m
     public int m() {
        return 0;
    }

 |--VARIABLE_DEF -> VARIABLE_DEF [4:4]
        |   |--MODIFIERS -> MODIFIERS [4:4]
        |   |--TYPE -> TYPE [4:4]
        |   |   `--LITERAL_INT -> int [4:4]
        |   |--IDENT -> x [4:8]
        |   `--SEMI -> ; [4:9]
        |--METHOD_DEF -> METHOD_DEF [6:3]
        |   |--MODIFIERS -> MODIFIERS [6:3]
        |   |   |--SINGLE_LINE_COMMENT -> // [5:4]
        |   |   |   `--COMMENT_CONTENT ->  m\r\n [5:6]
        |   |   `--LITERAL_PUBLIC -> public [6:3]

@romani @nrmancuso @rnveach
do we have any check that is aware of comments using AST-based logic not file content to take it as a reference?

[mahfouz72]

I will work more on this method. I believe that there are more cases that need to be handled to get the first comment token

[mahfouz72]

Github, generate report

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/3a9b4f9_2024191033/reports/diff/index.html

[mahfouz72]

dealing with comment tokens in the AST was weird to me. why it is under modifiers of the next subtree. is there a reason to build AST like this?

@nrmancuso ping

[nrmancuso]

dealing with comment tokens in the AST was weird to me. why it is under modifiers of the next subtree. is there a reason to build AST like this?

Yeah, comment placement in the AST is super weird, and has been for a long time. We have at least one issue on this. I can’t really imagine this check working well without getting file contents honestly.

IMO, this check should continue to use getFileContents

[mahfouz72]

Yeah, comment placement in the AST is super weird, and has been for a long time. We have at least one issue on this. I can’t really imagine this check working well without getting file contents honestly.

Yes, I tried a few times to continue this PR but things always get very messy and error-prone. I couldn't reach a good design. we should consider refactoring comments AST first before migrating this check to AST-based logic

IMO, this check should continue to use getFileContents

same and just wanted to know your opinion before taking a decision on this, should I close it or add blocked due to (#6233) or abandoned label

[nrmancuso]

Let’s make sure other maintainers are on board. @rnveach @romani ? We good to keep usage of getFileContents in this check?

[rnveach]

I am.

[nrmancuso]

We will not be able to complete this issue before GSOC. @mahfouz72 let's close this for now, and come back after summer. All your work will be saved via the link in the issue, and you can keep this branch in your fork to pick up later.