Skip to content

explicit generics annotation for constructor#734

Open
halotukozak wants to merge 7 commits intomasterfrom
explicit-generics-constructor
Open

explicit generics annotation for constructor#734
halotukozak wants to merge 7 commits intomasterfrom
explicit-generics-constructor

Conversation

@halotukozak
Copy link
Member

I've found that someone had used it for a constructor but it's no supported xd

@halotukozak halotukozak requested a review from ddworak August 21, 2025 17:28
@halotukozak halotukozak self-assigned this Aug 21, 2025
Copy link
Member

@ddworak ddworak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.apply case class instantiation should be handled as well for symmetry, this is even more common

@halotukozak
Copy link
Member Author

.apply case class instantiation should be handled as well for symmetry, this is even more common

true, I've even created a test for it xd. Will be done

@halotukozak
Copy link
Member Author

@sebaciv @ddworak how about this one?

# Conflicts:
#	analyzer/src/main/scala/com/avsystem/commons/analyzer/ExplicitGenerics.scala
Copilot AI review requested due to automatic review settings February 25, 2026 12:29
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the @explicitGenerics analyzer rule to support constructors in addition to methods. Previously, the annotation only enforced explicit type parameters on annotated methods. Now it also works on class and case class constructors, requiring explicit type arguments when instantiating annotated generic classes.

Changes:

  • Extended ExplicitGenerics analyzer to detect and report on constructors (via New nodes) with inferred type parameters
  • Added logic to detect case class apply methods when the case class itself is annotated, ensuring consistency
  • Added test fixtures and comprehensive test cases for the new constructor support

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
analyzer/src/main/scala/com/avsystem/commons/analyzer/ExplicitGenerics.scala Core implementation: added New case pattern matching, applyOfAnnotatedCompanion helper, and refactored error reporting
analyzer/src/test/scala/com/avsystem/commons/analyzer/TestUtils.scala Added test fixtures: GenericClass[T] and GenericCaseClass[T] annotated with @explicitGenerics
analyzer/src/test/scala/com/avsystem/commons/analyzer/ExplicitGenericsTest.scala Added four new test cases covering constructor and apply method scenarios with both inferred and explicit type arguments
analyzer/src/test/scala/com/avsystem/commons/analyzer/ImplicitValueClassesTest.scala Minor formatting cleanup: removed trailing commas and adjusted formatting style for consistency
Comments suppressed due to low confidence (3)

analyzer/src/main/scala/com/avsystem/commons/analyzer/ExplicitGenerics.scala:41

  • The companion class resolution logic here doesn't match the logic in applyOfAnnotatedCompanion. This line directly accesses pre.symbol.owner.companionClass, but the owner could be either a Module or ModuleClass. In applyOfAnnotatedCompanion, the code handles both cases by checking isModuleClass and isModule. This discrepancy could lead to getting NoSymbol or the wrong symbol in some cases. Consider extracting the companion class resolution into a helper function or reusing the companionCls value from applyOfAnnotatedCompanion.
              val targetSym = if (applyOfAnnotatedCompanion(pre.symbol)) pre.symbol.owner.companionClass else pre.symbol

analyzer/src/test/scala/com/avsystem/commons/analyzer/ExplicitGenericsTest.scala:74

  • The test case is missing a test for explicit type arguments on case class constructors using the new keyword (e.g., new TestUtils.GenericCaseClass[Int](123)). While the test at line 68-74 covers explicit type args for regular classes, it would be good to also verify that case classes work correctly with explicit type parameters when using new.
  test("explicit in constructor should not be rejected") {
    assertNoErrors(scala"""
             |import com.avsystem.commons.analyzer.TestUtils
             |
             |val x = new TestUtils.GenericClass[Int]()
             |""".stripMargin)
  }

analyzer/src/test/scala/com/avsystem/commons/analyzer/ExplicitGenericsTest.scala:66

  • Missing test case for case class apply method with explicit type arguments (e.g., TestUtils.GenericCaseClass[Int](123)). The test suite covers the error case (inferred type args in apply) but doesn't verify that the non-error case (explicit type args in apply) works correctly.
  test("inferred in apply when constructor marked should be rejected") {
    assertErrors(
      1,
      scala"""
             |import com.avsystem.commons.analyzer.TestUtils
             |
             |val x = TestUtils.GenericCaseClass(123)
             |""".stripMargin,
    )
  }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

4 participants