Refactor: remove detectioncontext instanceof

[Sanchay117]

Fix : #165

Summary

Change all translate function signatures across the translation layer to accept DetectionContext (concrete abstract class) instead of IDetectionContext (interface). This eliminates unnecessary instanceof DetectionContext runtime type checks.

Changes

Core Interface & Abstract Class

• IContextTranslation<T>: Changed translate() parameter from IDetectionContext → DetectionContext
• ITranslator<R,T,S,P>: Changed abstract translate() parameter and translateStore() local variable to use DetectionContext (with a cast at the DetectionStore boundary)

Engine Model Hierarchy

• PRNGContext: Now extends DetectionContext instead of directly implementing IDetectionContext
• ProtocolContext: Now extends DetectionContext instead of directly implementing IDetectionContext

This brings all context types into the DetectionContext hierarchy, making the cast in ITranslator.translateStore() safe.

Language Translators (4 modules)

Module	Files Modified
Java	JavaTranslator, JavaAbstractLibraryTranslator, JavaCipherContextTranslator, JavaDigestContextTranslator, JavaSignatureContextTranslator, JavaKeyContextTranslator, JavaMacContextTranslator, JavaKeyAgreementContextTranslator, JavaAlgorithmParameterContextTranslator, JavaSecretKeyContextTranslator, JavaPRNGContextTranslator, JavaProtocolContextTranslator
Python	PythonTranslator, PycaCipherContextTranslator, PycaDigestContextTranslator, PycaKeyAgreementContextTranslator, PycaKeyDerivationContextTranslator, PycaMacContextTranslator, PycaPrivateKeyContextTranslator, PycaPublicKeyContextTranslator, PycaSecretContextTranslator, PycaSecretKeyContextTranslator, PycaSignatureContextTranslator
Go	GoTranslator, GoCipherContextTranslator, GoDigestContextTranslator, GoKeyContextTranslator, GoMacContextTranslator, GoPRNGContextTranslator, GoProtocolContextTranslator, GoSignatureContextTranslator
C#	CSharpTranslator, CSharpCipherContextTranslator, CSharpDigestContextTranslator, CSharpKeyContextTranslator, CSharpMacContextTranslator

What was removed

All instanceof DetectionContext context checks in translator methods, replaced with direct usage of the detectionContext parameter. For example:

-if (value instanceof ValueAction<Tree> valueAction
-        && detectionContext instanceof DetectionContext context) {
-    String kind = context.get("kind").orElse("");
+if (value instanceof ValueAction<Tree> valueAction) {
+    String kind = detectionContext.get("kind").orElse("");

Testing

• All existing tests pass

[n1ckl0sk0rtge]

Thanks for picking up #165 — getting the instanceof DetectionContext cast out of the translator layer is the right direction. The refactor lands the easy half (signature swap + cast removal), but stops short of the goal the issue points at: once translators stop needing the cast, IDetectionContext itself, ISupportKind, and the typed kind fields on KeyContext / SignatureContext / ProtocolContext should all be gone — they only existed to support the indirection this PR is removing. As it stands the PR adds a duplicate kind entry to ProtocolContext.properties that no translator reads, leaves instanceof ProtocolContext in the Go translator, keeps the typed kind field as the de-facto source of truth, and still routes everything through IDetectionContext so an unchecked downcast had to be added at the translator boundary. Net result is more duplication than the codebase had before. Requesting changes to land the full cleanup — outline below.

Linked Issue

#165 — "Remove detectionContext instanceof DetectionContext context from the translation" (OPEN). Acceptance: partially met — instanceof DetectionContext is gone from translators, but the indirection that motivated those checks (the IDetectionContext / ISupportKind interfaces) is still in place, and one instanceof DetectionContext site was missed.

Summary of findings

• 3 MUST FIX — delete IDetectionContext, finish the property-map unification (drop kind fields, delete ISupportKind), remove the missed instanceof DetectionContext in DetectionStoreLogger.
• 1 SHOULD FIX — no test guards the new invariants.
• 2 SUGGESTIONS — map idiom consistency, document the breaking change in the PR body.

---

MUST FIX

1. Delete IDetectionContext entirely — the goal of the refactor

IDetectionContext only existed because translators historically received "some context" and had to ask "is it actually a DetectionContext?" before they could read properties. Once translator signatures take DetectionContext directly (which this PR already does), the interface has no users that can't be served by DetectionContext itself. Keeping it forces an unchecked downcast in ITranslator.translateStore (added by this PR) and leaves redundant implements IDetectionContext declarations on six concrete contexts.

Concrete steps:

• Delete engine/src/main/java/com/ibm/engine/model/context/IDetectionContext.java.
• DetectionContext.java:27 — drop implements IDetectionContext; declare type() directly on the abstract class with return type Class<? extends DetectionContext>.
• All concrete contexts' type() signatures — CipherContext, DigestContext, KeyContext, KeyAgreementContext, KeyDerivationFunctionContext, MacContext, PRNGContext, PrivateKeyContext, ProtocolContext, PublicKeyContext, SecretKeyContext, SignatureContext, AlgorithmParameterContext — change Class<? extends IDetectionContext> → Class<? extends DetectionContext>. Remove the redundant implements IDetectionContext declaration from each (KeyContext, KeyDerivationFunctionContext, SecretKeyContext, PrivateKeyContext, PublicKeyContext, SignatureContext all still carry it).
• engine/src/main/java/com/ibm/engine/detection/DetectionStore.java — getDetectionValueContext() and related fields should return DetectionContext. This eliminates the unchecked (DetectionContext) cast in mapper/src/main/java/com/ibm/mapper/ITranslator.java that this PR added — no cast needed, the type matches at the source.
• engine/src/main/java/com/ibm/engine/rule/IDetectionRule.java — detectionValueContext() should return DetectionContext.
• Detection rules under java/src/main/java/com/ibm/plugin/rules/detection/bc/... — BcDigests, BcBlockCipher, BcBlockCipherEngine, BcAsymCipherEngine, BcOAEPEncoding, BcISO9796d1Encoding, etc. all take @Nullable IDetectionContext detectionValueContext parameters. Swap to @Nullable DetectionContext and update the local IDetectionContext context = … lines (~12 sites in 8 files).
• Test files — java/src/test/java/com/ibm/plugin/rules/issues/DuplicateDependingRules2Test.java:49, DuplicateParametersFinding2Test.java:47, DetectionRuleMatchingExactTypesTest.java:71, DetectionRuleMatchingExactTypesExceptParametersTest.java:71 instantiate new IDetectionContext() { ... } anonymous classes. Switch to anonymous subclasses of DetectionContext (e.g., new DetectionContext(Map.of()) { @Override public Class<? extends DetectionContext> type() { return …; } }).

After this, the abstract-class boundary is the only contract, the cast in ITranslator.translateStore disappears (closes the latent ClassCastException risk), and the unchecked-downcast warning is gone.

2. Complete the property-map unification — drop the typed kind fields, route translators through get("kind"), delete ISupportKind

The PR added super(Map.of("kind", kind.name())) to ProtocolContext so the property is reachable via the abstract-class API, but no translator was migrated to use it and no other ISupportKind context was touched. The result is two sources of truth in ProtocolContext plus the very instanceof <ConcreteContext> cast pattern this refactor was supposed to eliminate.

KeyContext and SignatureContext already mark their typed kind field and kind() accessor as @Deprecated(since = "1.3.0") with the comment "use a property map instead" — the codebase already committed to this direction. This PR is the moment to finish it.

Step-by-step:

a. engine/.../ProtocolContext.java — drop field, accessor, and the ISupportKind declaration.

• Remove @Nonnull private final ProtocolContext.Kind kind; (line 32).
• Remove this.kind = … from both constructors (lines 36, 41).
• Remove @Override public Kind kind() { return this.kind; } (lines 53–56).
• Drop implements ISupportKind<ProtocolContext.Kind> from the class declaration (line 25). The Kind enum stays — translators still decode the string into it.
• For shape consistency with the other contexts, add public ProtocolContext(@Nonnull Map<String, String> properties) { super(properties); }.

b. engine/.../KeyContext.java — delete the deprecated field, accessor, and deprecated KeyContext(Kind) constructor; drop implements ISupportKind.

• Remove @Nonnull private final Kind kind;, the deprecated KeyContext(@Nonnull Kind kind) constructor, the deprecated kind() method, and the this.kind = Kind.NONE; lines from the remaining constructors.
• Drop implements ISupportKind<KeyContext.Kind> from the class declaration. The Kind enum stays.

c. engine/.../SignatureContext.java — same shape as KeyContext.

d. Delete engine/src/main/java/com/ibm/engine/model/context/ISupportKind.java. After (a)–(c) it has no implementers.

e. Translator call sites — switch to detectionContext.get("kind"):

• java/src/main/java/com/ibm/plugin/translation/translator/contexts/JavaProtocolContextTranslator.java:52 — replace

  final ProtocolContext.Kind kind = ((ProtocolContext) detectionContext).kind();

  with

  final ProtocolContext.Kind kind = detectionContext.get("kind")
      .map(ProtocolContext.Kind::valueOf)
      .orElse(ProtocolContext.Kind.NONE);

• go/src/main/java/com/ibm/plugin/translation/translator/contexts/GoProtocolContextTranslator.java:59–60 — drop the && detectionContext instanceof ProtocolContext protocolContext narrowing and read the kind from the map the same way. This site is the clearest demonstration that the original refactor goal isn't yet achieved: the instanceof cast is still there, just for a concrete subtype.

f. engine/src/main/java/com/ibm/engine/utils/DetectionStoreLogger.java:145–148 — the else if (detectionValueContext instanceof ISupportKind<?>) branch becomes dead once the interface is deleted. Remove it, or replace with a detectionValueContext.get("kind") lookup if the log line is worth keeping.

g. java/src/test/java/com/ibm/plugin/rules/detection/jca/keyfactory/JcaKeyFactoryGenerateTest.java:80 — the only production-tree caller of KeyContext.kind(). Update to assert the property map directly.

After this, every *Context is a pure marker subclass of DetectionContext whose only state is properties; translators consume kind uniformly via get("kind"); the instanceof <X>Context cast pattern is gone from the translator layer; and ISupportKind is deleted.

3. Dead instanceof DetectionContext branch left behind

engine/src/main/java/com/ibm/engine/utils/DetectionStoreLogger.java:143

After this PR every IDetectionContext extends DetectionContext (and after MUST FIX #1 the interface is gone entirely), so the guard is unreachable. The PR title promises removal of these checks; this one was missed. Drop the guard and inline the body. Combine with the ISupportKind branch removal in MUST FIX #2f — same file.

---

SHOULD FIX

4. No test guards the new invariants

The cast removal (MUST FIX #1) and the kind-via-map migration (MUST FIX #2) are both new invariants with no targeted test. Existing translator tests (GoCryptoTLSTest, SSLContextGetInstanceTest, etc.) will catch regressions transitively. Add at minimum:

• A unit test asserting ProtocolContext.get("kind") returns the seeded value for both constructors.
• A unit test asserting DetectionStore.getDetectionValueContext() returns DetectionContext (or that the method's static return type compiles as DetectionContext).

---

SUGGESTIONS

5. Map.of() vs new HashMap<>() idiom divergence

PRNGContext and ProtocolContext use immutable Map.of(...); siblings (CipherContext, DigestContext, MacContext) use mutable new HashMap<>(). DetectionContext exposes no mutator, so there's no runtime bug — pure stylistic split. Standardize on Map.of(...) to match the direction in this PR.

6. Document the source-incompatible change

IContextTranslation.translate(...) and ITranslator.translate(...) signatures changed from IDetectionContext to DetectionContext; once MUST FIX #1 lands, IDetectionContext is deleted outright. Both are breaking for any third-party plugin that implements these interfaces. Add a "Breaking change" note to the PR description so it surfaces in release notes.

---

Recommendation

Request changes. The PR title says "remove detectioncontext instanceof" but the result is additional indirection: a property-map entry no one reads, a typed field no one removed, an unchecked cast at the translator boundary, and instanceof ProtocolContext still in the Go translator. The right end-state for issue #165 is the one the codebase has already telegraphed (deprecation notices, the Map<String, String> constructor on KeyContext / SignatureContext): one DetectionContext abstract class, property-map state, no IDetectionContext, no ISupportKind, no concrete-context casts in translators. Happy to review again once the cleanup is folded in.

[Sanchay117]

Thank you for the thorough and extremely constructive feedback.

I have fully resolved every single point raised in the feedback. Here is the breakdown of the changes that have been implemented on top of the existing PR:

---

MUST FIX

1. Delete IDetectionContext entirely

• Deleted File: engine/src/main/java/com/ibm/engine/model/context/IDetectionContext.java.
• Base Class Migration: DetectionContext.java is now the concrete abstract base class and does not implement any interface. The type() signature returns Class<? extends DetectionContext>.
• Concrete Context Updates: Standardized CipherContext, DigestContext, KeyContext, KeyAgreementContext, KeyDerivationFunctionContext, MacContext, PRNGContext, PrivateKeyContext, ProtocolContext, PublicKeyContext, SecretKeyContext, SignatureContext, and AlgorithmParameterContext to extend DetectionContext directly and return Class<? extends DetectionContext>.
• Cast Elimination: Eliminated the unsafe (DetectionContext) downcast from ITranslator.translateStore() in the translation layer. Types are now fully aligned at the source.
• Engine Rules & Builders: Updated all rule definitions (IDetectionRule, DetectionRule, MethodDetectionRule, DetectionRuleBuilderImpl) and BouncyCastle rule files (~8 files) to accept DetectionContext.
• Test Alignment: Updated all anonymous test context instantiations in issue tests to subclass DetectionContext directly.

2. Complete Property-Map Unification

• Deleted File: engine/src/main/java/com/ibm/engine/model/context/ISupportKind.java (no longer needed).
• Context Cleanup: Removed the deprecated typed kind fields, constructors, and kind() accessor methods from ProtocolContext, KeyContext, and SignatureContext.
• Map Consolidation: Conveniences such as new ProtocolContext(ProtocolContext.Kind.TLS) remain but now cleanly seed the property map (Map.of("kind", kind.name())) under the hood, standardizing state.
• Translator Refactoring:
  • Updated JavaProtocolContextTranslator.java to fetch the kind via detectionContext.get("kind").map(ProtocolContext.Kind::valueOf).
  • Removed instanceof ProtocolContext from GoProtocolContextTranslator.java and migrated it to the property map lookup as well.

3. Dead instanceof DetectionContext branch removal

• Logger Cleaned: In DetectionStoreLogger.java, removed the dead instanceof ISupportKind and instanceof DetectionContext branches. Logger now queries detectionContext.get("kind") directly, keeping it lean.

---

SHOULD FIX

4. Added targeted unit tests for the new invariants

• New Test File: Added ContextTest.java.
• Assertions:
  • Validates that ProtocolContext, KeyContext, and SignatureContext constructors map kinds correctly to the internal property map.
  • Verifies that empty or Kind.NONE instances correctly yield an empty map.
  • Verifies that retrieving "kind" from the map works seamlessly via the new abstract-class property map API.

---

SUGGESTIONS

5. Map.of() vs new HashMap<>() Idiom Consistency

• Standardized CipherContext, DigestContext, and MacContext to use immutable Map.of(...) instead of new HashMap<>() to align with the rest of the codebase.

6. Document the Breaking Change

• Added a note regarding the removal of IDetectionContext and the change of ITranslator parameter signatures to the PR body. Any custom downstream translator implementation will need to update their signature to accept the abstract DetectionContext directly.

---

Verification

• Compilation: Ran mvn clean test-compile successfully.
• Tests: Ran mvn clean test successfully across all 12 modules.
• Static Analysis: Verified 0 occurrences of IDetectionContext, ISupportKind, instanceof DetectionContext, or instanceof ProtocolContext remain in the codebase.