checker-framework icon indicating copy to clipboard operation
checker-framework copied to clipboard

Use the compound checker implemention to implement the Aggregate Checker.

Open smillst opened this issue 1 year ago • 1 comments

  • Moves the code that handles subcheckers into SourceChecker.
  • Use the this code to instead of special casing the Aggregate Checker to avoid code duplication. (This means that the Aggregate Checker now it interleaves the subcheckers error messages.

(This is modified from #6723. Here's a branch with the changes in that pull request and this one: https://github.com/smillst/checker-framework/tree/decouple-rlc-cm-new-cc)

smillst avatar Aug 06 '24 19:08 smillst

Could you please update the documentation in the manual as well?

https://checkerframework.org/manual/#creating-bundling-multiple-checkers talks of "aggregate checkers" and "compound checkers".

How does this fit in?

I answered this in the pull request description:

  • Adds a new class called CompositeChecker that is a subclass of SourceChecker. It is a replacement for the AggregateChecker. (Though, unlike the Aggregate Checker, it interleaves the subcheckers error messages.) [[Should we just keep the Aggregate Checker name?]]

I will update the documentation once we decided that it makes sense to rename the Aggregate Checker to the Composite Checker. Other than the rename, the only thing that needs to be changed about the manual is that the Aggregate checker interleaves the subcheckers error messages. (Also, as I noted in the pull request description, this needs a change log entry.)

smillst avatar Aug 08 '24 14:08 smillst

Can you please update the manual? I would like to read the documentation before the code.

From a comment comment that refers to text that is not in the pull request description, the pull request seems to replace AggregateChecker by CompositeChecker, but the title says "implement the Aggregate Checker".

What is the motivation to rename "Aggregate Checker" to "Compound Checker"? I'm a bit concerned that "Composite Checker" and "Compound Checker" are exteremely similar names, but will refer to rather different ways to combine checkers.

mernst avatar Sep 09 '24 13:09 mernst

Can you please update the manual? I would like to read the documentation before the code.

From a comment comment that refers to text that is not in the pull request description, the pull request seems to replace AggregateChecker by CompositeChecker, but the title says "implement the Aggregate Checker".

What is the motivation to rename "Aggregate Checker" to "Compound Checker"? I'm a bit concerned that "Composite Checker" and "Compound Checker" are exteremely similar names, but will refer to rather different ways to combine checkers.

I feel like you are looking at stale version of this pull request. Originally this pull request added a Composite Checker, because that's what the code in https://github.com/typetools/checker-framework/pull/6723 did. We discussed this and decided not to add the Composite Checker. Because it's a confusing name. Instead, I just changed the implementation of the Aggregate Checker to use the same implementation of compound checker. To do this I moved code from the BaseTypeChecker to SourceChecker so that AggregateChecker can use it. Then I did update the manual, but not much changed for the user point of view because I only really change how the Aggregate Checker was implement. The only user visible changes to the Aggregate Checker are interleaved error messages and a different method needs to be overridden.

smillst avatar Sep 09 '24 16:09 smillst