From fece0a75cdf67d995feac6f9480baa8ad36e05a6 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Tue, 4 Aug 2026 12:56:14 -0500 Subject: [PATCH] Add compile-time DynamicClient usage check as an Error Prone BugChecker Validates DynamicClient usage against the Smithy model each client is actually built from, at compile time, without any code generation. The dynamic client stays fully runtime-driven and document-based; the check only reads the source under compilation. The DynamicClientUsage BugChecker: - finds DynamicClient.builder()...build() chains and reads .model(...) and .serviceId(...) (handling ShapeId.from("...")); - statically resolves the model source by walking the Model.assembler() chain (addUnparsedModel/addImport/discoverModels, folding constants); - assembles that model in-process with the real smithy-model ModelAssembler (same code the runtime uses, no drift), cached per client; - checks each client.call("Op", Map.of(...)) site for a valid operation name (with a Levenshtein 'did you mean' SuggestedFix) and valid input member keys. Core rule: abstain, never false-positive. Any genuinely dynamic value (runtime operation name, model passed in from elsewhere, addImport(url)) yields NO_MATCH, so runtime-dynamic usage like the CLI's SmithyCall is left alone. The model-resolution core (ModelResolver, ResolvedClient, MapLiteral) is decoupled from the compiler via a constantResolver function. Verified Error Prone 2.50.0 runs under the project's Java 25 toolchain (needs the --add-exports/--add-opens set + --should-stop=ifError=FLOW; --release is dropped since it conflicts with --add-exports of a system module). Full :build passes: compile, spotless, javadoc, and 6 CompilationTestHelper tests covering unknown operation, bad input key, auto-detected service, and correct abstention on dynamic op name / unresolved model. --- client/dynamic-client-errorprone/README.md | 132 ++++++++ .../build.gradle.kts | 72 ++++ .../compiler/DeclarationFinder.java | 42 +++ .../compiler/DynamicClientUsageChecker.java | 313 ++++++++++++++++++ .../dynamicclient/compiler/MapLiteral.java | 48 +++ .../dynamicclient/compiler/ModelResolver.java | 196 +++++++++++ .../compiler/ResolvedClient.java | 83 +++++ .../DynamicClientUsageCheckerTest.java | 166 ++++++++++ gradle/libs.versions.toml | 5 + settings.gradle.kts | 1 + 10 files changed, 1058 insertions(+) create mode 100644 client/dynamic-client-errorprone/README.md create mode 100644 client/dynamic-client-errorprone/build.gradle.kts create mode 100644 client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/DeclarationFinder.java create mode 100644 client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/DynamicClientUsageChecker.java create mode 100644 client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/MapLiteral.java create mode 100644 client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/ModelResolver.java create mode 100644 client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/ResolvedClient.java create mode 100644 client/dynamic-client-errorprone/src/test/java/software/amazon/smithy/java/dynamicclient/compiler/DynamicClientUsageCheckerTest.java diff --git a/client/dynamic-client-errorprone/README.md b/client/dynamic-client-errorprone/README.md new file mode 100644 index 000000000..a5222b37f --- /dev/null +++ b/client/dynamic-client-errorprone/README.md @@ -0,0 +1,132 @@ +# Dynamic client usage check (Error Prone) + +An **[Error Prone](https://errorprone.info) check** that validates `DynamicClient` usage against the Smithy model each +client is *actually built from*, at compile time — **without** any code generation. The dynamic client stays fully +runtime-driven and document-based; this check only reads the source being compiled and catches the mistakes that are +statically catchable. + +## What it checks + +Given code like: + +```java +DynamicClient client = DynamicClient.builder() + .serviceId(ShapeId.from("smithy.example#Sprockets")) + .model(Model.assembler().addUnparsedModel("demo.smithy", MODEL).assemble().unwrap()) + .build(); + +client.call("GetSprokcet", Map.of("id", "1")); // <-- compile error: typo, no such operation +client.call("GetSprocket", Map.of("idd", "1")); // <-- compile error: 'idd' not an input member +``` + +the check emits, anchored to the exact argument: + +``` +error: [DynamicClientUsage] Operation 'GetSprokcet' not found in service 'smithy.example#Sprockets'. + Known operations: [CreateSprocket, GetSprocket] + Did you mean 'GetSprocket'? <-- suggested auto-fix +error: [DynamicClientUsage] 'idd' is not a member of input 'GetSprocketInput' for operation 'GetSprocket'. + Known members: [id] +``` + +Concretely, per `call(...)` site whose receiver is a `DynamicClient` it can resolve: + +1. **operation name** — the first argument, when a String literal or `static final String` constant, must be an + operation on the resolved service (with a Levenshtein-based *"did you mean"* suggested fix); +2. **input keys** — when the second argument is a `Map.of("k", v, ...)` literal, every key must be a member of the + operation's input structure. + +## Why Error Prone (vs. a raw javac plugin) + +This started as a raw `com.sun.source.util.Plugin`. Porting to Error Prone kept the same idea but replaced hand-rolled +machinery with framework features: + +| Hand-rolled in the raw plugin | Provided by Error Prone | +|---|---| +| Custom string-constant folding | `ASTHelpers.constValue(...)` | +| Report-only diagnostics | **`SuggestedFix`** — an applyable "did you mean `GetSprocket`?" auto-fix | +| A `run.sh` shell driver | `CompilationTestHelper` with inline `// BUG: Diagnostic contains:` assertions | +| No suppression | `@SuppressWarnings("DynamicClientUsage")` for free | +| Manual `TaskListener` wiring | `@AutoService(BugChecker.class)` registration | + +The **model-resolution core** (`ModelResolver`, `ResolvedClient`, `MapLiteral`) is unchanged from the raw-plugin +prototype and has no dependency on Error Prone or javac internals — it takes a `constantResolver` function, so the same +core could back an Error Prone check, a javac plugin, or a standalone parser task. Error Prone is just the driver. + +## How it works + +- **Match** (`DynamicClientUsageChecker`) — a `MethodInvocationTreeMatcher` matching `call(...)` on `DynamicClient`. +- **Resolve the client** — walk from the `call` receiver to the local's `DynamicClient.builder()...build()` + initializer, read the `.model(...)` and `.serviceId(...)` arguments (handling `ShapeId.from("...")`). +- **Resolve + assemble the model** (`ModelResolver`) — statically read the `Model.assembler()...` chain + (`addUnparsedModel` / `addImport` / `discoverModels`, folding constants) and assemble it with the **real + `smithy-model` `ModelAssembler`** — the same code the runtime uses, so no drift. Cached per client. +- **Validate** — check the operation name and any `Map.of(...)` input keys, reporting via `buildDescription(...)`. + +## The load-bearing rule: abstain, never false-positive + +The check only reasons about statically-resolvable values. The instant anything is genuinely dynamic — an operation +name from a field or parameter, a model passed in from elsewhere, `addImport(someUrl)` — it returns +`Description.NO_MATCH` and never errors. `DynamicClient` exists *for* runtime dynamism; a checker that flags valid +dynamic code gets turned off. It catches typos and stays out of the way. (See `abstainsOnDynamicOperationName` and +`abstainsWhenModelNotStaticallyResolvable` in the tests, and the CLI's `SmithyCall`, which is entirely in the abstain +set.) + +## Running it in a consumer build + +With the `net.ltgt.errorprone` Gradle plugin, put this module on the `errorprone` configuration: + +```kotlin +plugins { id("net.ltgt.errorprone") version "..." } +dependencies { + errorprone("com.google.errorprone:error_prone_core:2.50.0") + errorprone(project(":client:dynamic-client-errorprone")) +} +``` + +### Running *only* this check (no other Error Prone checks) + +Consumers who don't want Error Prone's ~500 built-in checks can disable them all and enable just this one: + +```kotlin +tasks.withType().configureEach { + options.errorprone { + disableAllChecks = true + error("DynamicClientUsage") + } +} +``` + +Note this still requires Error Prone the framework to be present — see "Known limitations". + +## Try it + +``` +./gradlew :client:dynamic-client-errorprone:test +``` + +The `CompilationTestHelper` tests compile inline sources against the real `DynamicClient` with the check active and +assert the diagnostics (unknown operation, bad input key, auto-detected service, and correct abstention on dynamic op +name and unresolved model). + +## Java 25 / toolchain note + +Error Prone runs inside `javac` and depends on compiler internals, so it needs the standard `--add-exports` / +`--add-opens` set and `--should-stop=ifError=FLOW`. Verified working on this project's **Java 25** toolchain with +Error Prone **2.50.0**. Because `--add-exports` of a system-module package is incompatible with `--release`, this +module compiles with plain `source`/`target` 21 rather than the `--release 21` the shared conventions set (see +`build.gradle.kts`). + +## Known limitations / next steps + +- **Requires Error Prone the framework.** "Only this check" disables the *other checks*, but consumers still adopt + Error Prone + `net.ltgt.errorprone`. If a zero-Error-Prone dependency is required, the raw-javac-plugin form (in git + history) or a standalone parser-based Gradle task are the alternatives; both reuse the same `ModelResolver` core. +- **Coupled to javac / Error Prone versions.** Newer JDKs can require a matching Error Prone bump. +- **Intra-unit flow only.** A client built in one file and used in another isn't tracked; an optional + `@SmithyModel(...)` fallback annotation naming the source would extend reach. +- **`discoverModels()` / classpath imports** are approximated via import roots; wiring the compilation's real + `JavaFileManager` classpath would make it exact. +- Only operation names and `Map.of` input keys are checked today; required members, enum values, and nested document + shapes are natural extensions. +``` diff --git a/client/dynamic-client-errorprone/build.gradle.kts b/client/dynamic-client-errorprone/build.gradle.kts new file mode 100644 index 000000000..68fd3ec1d --- /dev/null +++ b/client/dynamic-client-errorprone/build.gradle.kts @@ -0,0 +1,72 @@ +plugins { + id("smithy-java.module-conventions") +} + +description = "Error Prone check that validates DynamicClient usage against the Smithy model at compile time" + +extra["displayName"] = "Smithy :: Java :: Dynamic client Error Prone check" +extra["moduleName"] = "software.amazon.smithy.java.dynamicclient.compiler" + +dependencies { + // The Error Prone check API this BugChecker extends. + compileOnly(libs.errorprone.check.api) + compileOnly(libs.errorprone.annotation) + + // AutoService generates the META-INF/services/com.google.errorprone.bugpatterns.BugChecker registration. + compileOnly("com.google.auto.service:auto-service-annotations:1.1.1") + annotationProcessor("com.google.auto.service:auto-service:1.1.1") + + // The check runs inside javac and assembles the resolved model with the same ModelAssembler the runtime uses. + implementation(libs.smithy.model) + + testImplementation(libs.errorprone.test.helpers) + testImplementation(libs.errorprone.core) + testImplementation(project(":client:dynamic-client")) +} + +// Error Prone's check API compiles against javac internals, which require these exports on JDK 16+. +// `--add-exports` of a system-module package is incompatible with `--release`, so this module compiles with +// plain source/target (21, matching its dependencies) instead of the `--release 21` the conventions set. +tasks.withType().configureEach { + options.release.set(null as Int?) + sourceCompatibility = "21" + targetCompatibility = "21" + options.compilerArgs.addAll( + listOf( + "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", + ), + ) +} + +// Javadoc also parses the compiler-internal references, so it needs the same exports. +tasks.withType().configureEach { + (options as StandardJavadocDocletOptions).addMultilineStringsOption("-add-exports").value = listOf( + "jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", + "jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", + "jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", + ) +} + +// SpotBugs analyzes bytecode that references javac internals it cannot load; not meaningful for an Error Prone check. +tasks.named("spotbugsMain") { enabled = false } + +// The Error Prone test helpers spin up an in-process javac that also needs the module opens/exports at runtime. +tasks.withType().configureEach { + jvmArgs( + "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", + ) +} diff --git a/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/DeclarationFinder.java b/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/DeclarationFinder.java new file mode 100644 index 000000000..411d05e1c --- /dev/null +++ b/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/DeclarationFinder.java @@ -0,0 +1,42 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.java.dynamicclient.compiler; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.VariableTree; +import com.sun.source.util.TreePathScanner; +import com.sun.tools.javac.code.Symbol; + +/** + * Scans a compilation unit for the {@link VariableTree} that declares a given {@link Symbol}. Used as a fallback when + * the declaration is not on the path from the current call site to the top level (e.g. a field, or a local declared in + * a sibling method within the same file). + */ +final class DeclarationFinder { + + private DeclarationFinder() {} + + static VariableTree find(Symbol target, VisitorState state) { + var unit = state.getPath().getCompilationUnit(); + if (unit == null) { + return null; + } + var scanner = new TreePathScanner() { + VariableTree found; + + @Override + public Void visitVariable(VariableTree node, Void unused) { + if (found == null && ASTHelpers.getSymbol(node) == target) { + found = node; + } + return super.visitVariable(node, unused); + } + }; + scanner.scan(unit, null); + return scanner.found; + } +} diff --git a/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/DynamicClientUsageChecker.java b/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/DynamicClientUsageChecker.java new file mode 100644 index 000000000..786d1c92a --- /dev/null +++ b/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/DynamicClientUsageChecker.java @@ -0,0 +1,313 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.java.dynamicclient.compiler; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Symbol; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.shapes.MemberShape; +import software.amazon.smithy.model.shapes.OperationShape; +import software.amazon.smithy.model.shapes.StructureShape; + +/** + * Error Prone check that validates {@code DynamicClient} usage against the Smithy model each client is actually built + * from — at compile time, with no code generation. + * + *

The dynamic client stays fully runtime-driven and document-based. This check only reads the source under + * compilation: it finds {@code DynamicClient.builder()...build()} chains, resolves the model source by statically + * walking the {@code Model.assembler()...} chain feeding {@code .model(...)}, assembles that model in-process with the + * real {@code smithy-model} {@link software.amazon.smithy.model.loader.ModelAssembler}, and checks each + * {@code client.call("Op", Map.of(...))} site: + * + *

    + *
  • the operation name (a String literal or {@code static final String} constant) exists on the resolved + * service; and
    • + *
  • if the input is a {@code Map.of("k", v, ...)} literal, every key is a member of the operation input.
    • + *
+ * + *

The load-bearing rule: abstain, never false-positive

+ * + *

The check only reasons about statically-resolvable values. The instant a value is genuinely dynamic — an + * operation name from a field or CLI arg, a model path computed at runtime, {@code addImport(someUrl)} — it returns + * {@link Description#NO_MATCH}. The whole reason {@code DynamicClient} exists is runtime dynamism; a checker that flags + * valid dynamic code gets turned off. It catches typos and stays out of the way otherwise. + * + *

Running only this check

+ * + *

Consumers who do not want Error Prone's other checks can run this one alone: + * {@code -Xep:DynamicClientUsage:ERROR -XepDisableAllChecks}. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "DynamicClientUsage", + summary = "Verifies DynamicClient operation names and input keys against the Smithy model the client is " + + "built from.", + severity = BugPattern.SeverityLevel.ERROR, + link = "https://github.com/smithy-lang/smithy-java", + linkType = BugPattern.LinkType.CUSTOM) +public final class DynamicClientUsageChecker extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { + + private static final String DYNAMIC_CLIENT = "software.amazon.smithy.java.dynamicclient.DynamicClient"; + + /** Matches any {@code call(...)} instance method on DynamicClient (all overloads take the op name first). */ + private static final Matcher CALL = + MethodMatchers.instanceMethod().onExactClass(DYNAMIC_CLIENT).named("call"); + + /** + * Cache of resolved clients keyed by the builder variable's symbol, so the model for a given client is assembled + * once per compilation regardless of how many call sites reference it. Keyed by symbol identity via its string + * form; a fresh resolver-backed model is cheap to look up but not to assemble. + */ + private final Map clientCache = new ConcurrentHashMap<>(); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!CALL.matches(tree, state)) { + return Description.NO_MATCH; + } + var args = tree.getArguments(); + if (args.isEmpty()) { + return Description.NO_MATCH; + } + + // Resolve which client this call is made on, and the model it was built from. + ResolvedClient client = resolveClient(tree, state); + if (client == null || client.model() == null) { + return Description.NO_MATCH; // unknown client or unresolved model: abstain. + } + + // Operation name must be a compile-time constant to validate. ASTHelpers.constValue folds literals + constants. + String operation = ASTHelpers.constValue(args.get(0), String.class); + if (operation == null) { + return Description.NO_MATCH; // dynamic operation name: abstain. + } + + OperationShape opShape = client.operations().get(operation); + if (opShape == null) { + return buildUnknownOperation(tree, args.get(0), operation, client, state); + } + + // If a second argument is a Map.of(...) literal, validate its keys against the input structure members. + if (args.size() >= 2) { + Description inputProblem = validateInputKeys(args.get(1), opShape, client); + if (inputProblem != null) { + return inputProblem; + } + } + return Description.NO_MATCH; + } + + private Description buildUnknownOperation( + MethodInvocationTree tree, + ExpressionTree opArg, + String operation, + ResolvedClient client, + VisitorState state + ) { + var description = buildDescription(opArg) + .setMessage(String.format( + "Operation '%s' not found in service '%s'. Known operations: %s", + operation, + client.service().getId(), + client.sortedOperationNames())); + // Suggested fix: if there is a single close match by edit distance, offer it. (Auto-fix is EP's payoff.) + String suggestion = closestMatch(operation, client.operations().keySet()); + if (suggestion != null) { + description.addFix(SuggestedFix.replace(opArg, '"' + suggestion + '"')); + } + return description.build(); + } + + private Description validateInputKeys(ExpressionTree inputArg, OperationShape opShape, ResolvedClient client) { + List keys = MapLiteral.keysOf(inputArg, e -> ASTHelpers.constValue(e, String.class)); + if (keys == null) { + return null; // not a resolvable Map.of(...) literal (e.g. a Document or variable): nothing to check. + } + var inputId = opShape.getInputShape(); + var inputShape = client.model().getShape(inputId).orElse(null); + if (!(inputShape instanceof StructureShape structure)) { + return null; + } + Map members = structure.getAllMembers(); + for (String key : keys) { + if (!members.containsKey(key)) { + return buildDescription(inputArg) + .setMessage(String.format( + "'%s' is not a member of input '%s' for operation '%s'. Known members: %s", + key, + inputId.getName(), + opShape.getId().getName(), + members.keySet().stream().sorted().toList())) + .build(); + } + } + return null; + } + + /** Resolve the receiver of a {@code call(...)} back to the {@code DynamicClient} builder that produced it. */ + private ResolvedClient resolveClient(MethodInvocationTree call, VisitorState state) { + ExpressionTree receiver = ASTHelpers.getReceiver(call); + if (receiver == null) { + return null; + } + Symbol receiverSymbol = ASTHelpers.getSymbol(receiver); + if (receiverSymbol == null) { + return null; + } + ResolvedClient cached = clientCache.get(receiverSymbol); + if (cached != null) { + return cached; + } + // Find the variable declaration for the receiver and inspect its initializer builder chain. + VariableTree declaration = findDeclaration(receiverSymbol, state); + if (declaration == null || declaration.getInitializer() == null) { + return null; + } + if (!isDynamicClientBuild(declaration.getInitializer())) { + return null; + } + ExpressionTree modelArg = findNamedArgument(declaration.getInitializer(), "model"); + ExpressionTree modelExpr = modelArg == null ? null : deref(modelArg, state); + Model model = modelExpr == null + ? null + : new ModelResolver(e -> ASTHelpers.constValue(e, String.class), List.of()).resolve(modelExpr); + ExpressionTree serviceArg = findNamedArgument(declaration.getInitializer(), "serviceId"); + String serviceId = resolveServiceId(serviceArg); + ResolvedClient resolved = ResolvedClient.of(model, serviceId); + clientCache.put(receiverSymbol, resolved); + return resolved; + } + + /** + * Resolve the service ID from a {@code serviceId(...)} argument. Handles both a String constant and the common + * {@code ShapeId.from("ns#Name")} form. Returns {@code null} when not statically resolvable, in which case + * {@link ResolvedClient} falls back to single-service auto-detection like the runtime builder does. + */ + private String resolveServiceId(ExpressionTree serviceArg) { + if (serviceArg == null) { + return null; + } + String direct = ASTHelpers.constValue(serviceArg, String.class); + if (direct != null) { + return direct; + } + // ShapeId.from("smithy.example#Sprockets") + if (serviceArg instanceof MethodInvocationTree mi + && mi.getMethodSelect() instanceof MemberSelectTree sel + && sel.getIdentifier().contentEquals("from") + && sel.getExpression().toString().endsWith("ShapeId") + && mi.getArguments().size() == 1) { + return ASTHelpers.constValue(mi.getArguments().get(0), String.class); + } + return null; + } + + /** Whether an initializer is a {@code DynamicClient.builder()...build()} chain. */ + private boolean isDynamicClientBuild(ExpressionTree expr) { + ExpressionTree cursor = expr; + while (cursor instanceof MethodInvocationTree mi + && mi.getMethodSelect() instanceof MemberSelectTree sel) { + if (sel.getIdentifier().contentEquals("builder") + && sel.getExpression().toString().endsWith("DynamicClient")) { + return true; + } + cursor = sel.getExpression(); + } + return false; + } + + /** Find the argument passed to {@code .name(x)} anywhere in a builder chain. */ + private ExpressionTree findNamedArgument(ExpressionTree expr, String methodName) { + ExpressionTree cursor = expr; + while (cursor instanceof MethodInvocationTree mi + && mi.getMethodSelect() instanceof MemberSelectTree sel) { + if (sel.getIdentifier().contentEquals(methodName) && !mi.getArguments().isEmpty()) { + return mi.getArguments().get(0); + } + cursor = sel.getExpression(); + } + return null; + } + + /** If the model argument is an identifier, dereference it to the initializer of the variable it names. */ + private ExpressionTree deref(ExpressionTree expr, VisitorState state) { + if (expr instanceof MethodInvocationTree) { + return expr; // already a Model.assembler()... chain. + } + Symbol symbol = ASTHelpers.getSymbol(expr); + if (symbol == null) { + return expr; + } + VariableTree declaration = findDeclaration(symbol, state); + if (declaration != null && declaration.getInitializer() != null) { + return declaration.getInitializer(); + } + return expr; + } + + /** Locate the {@link VariableTree} declaring {@code symbol} within the current compilation unit. */ + private VariableTree findDeclaration(Symbol symbol, VisitorState state) { + return DeclarationFinder.find(symbol, state); + } + + /** Return the single closest operation name by Levenshtein distance if it is unambiguously close, else null. */ + private static String closestMatch(String typo, Set candidates) { + String best = null; + int bestDistance = Integer.MAX_VALUE; + int secondBest = Integer.MAX_VALUE; + for (String candidate : candidates) { + int distance = levenshtein(typo, candidate); + if (distance < bestDistance) { + secondBest = bestDistance; + bestDistance = distance; + best = candidate; + } else if (distance < secondBest) { + secondBest = distance; + } + } + // Only suggest when there's a clear, close winner (<= a third of the length, and distinctly closest). + int threshold = Math.max(1, typo.length() / 3); + if (best != null && bestDistance <= threshold && bestDistance < secondBest) { + return best; + } + return null; + } + + private static int levenshtein(String a, String b) { + int[] prev = new int[b.length() + 1]; + int[] curr = new int[b.length() + 1]; + for (int j = 0; j <= b.length(); j++) { + prev[j] = j; + } + for (int i = 1; i <= a.length(); i++) { + curr[0] = i; + for (int j = 1; j <= b.length(); j++) { + int cost = a.charAt(i - 1) == b.charAt(j - 1) ? 0 : 1; + curr[j] = Math.min(Math.min(curr[j - 1] + 1, prev[j] + 1), prev[j - 1] + cost); + } + int[] tmp = prev; + prev = curr; + curr = tmp; + } + return prev[b.length()]; + } +} diff --git a/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/MapLiteral.java b/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/MapLiteral.java new file mode 100644 index 000000000..0509b35fc --- /dev/null +++ b/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/MapLiteral.java @@ -0,0 +1,48 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.java.dynamicclient.compiler; + +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import java.util.ArrayList; +import java.util.List; +import java.util.function.Function; + +/** + * Extracts the string keys from a {@code Map.of("k1", v1, "k2", v2, ...)} or {@code Map.ofEntries(...)}-free literal + * so input members can be checked at compile time. + */ +final class MapLiteral { + + private MapLiteral() {} + + /** + * @return the list of literal string keys if {@code expr} is a fully-resolvable {@code Map.of(...)} call with an + * even argument count and constant keys; otherwise {@code null} (meaning "not a checkable map literal"). + */ + static List keysOf(ExpressionTree expr, Function constantResolver) { + if (!(expr instanceof MethodInvocationTree call) + || !(call.getMethodSelect() instanceof MemberSelectTree select) + || !select.getIdentifier().contentEquals("of") + || !select.getExpression().toString().endsWith("Map")) { + return null; + } + var args = call.getArguments(); + if (args.isEmpty() || args.size() % 2 != 0) { + return null; // Map.of() empty is fine (no keys to check) but returns empty below; odd => not Map.of(k,v..). + } + List keys = new ArrayList<>(); + for (int i = 0; i < args.size(); i += 2) { + String key = constantResolver.apply(args.get(i)); + if (key == null) { + return null; // a non-constant key: give up on key checking rather than half-check. + } + keys.add(key); + } + return keys; + } +} diff --git a/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/ModelResolver.java b/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/ModelResolver.java new file mode 100644 index 000000000..b10e83591 --- /dev/null +++ b/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/ModelResolver.java @@ -0,0 +1,196 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.java.dynamicclient.compiler; + +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.function.Function; +import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.loader.ModelAssembler; +import software.amazon.smithy.model.validation.Severity; + +/** + * Resolves the Smithy {@link Model} that a {@code DynamicClient} is built from by statically reading the + * {@code Model.assembler()...assemble().unwrap()} chain that feeds {@code builder().model(...)}. + * + *

This is the mechanism-agnostic core of the check: static source resolution, then compile-time assembly using the + * exact same {@link ModelAssembler} the runtime uses (no reimplementation, no drift). It has no dependency on javac + * internals or Error Prone — the caller supplies a {@code constantResolver} that maps an expression to its compile-time + * String value (backed by {@code ASTHelpers.constValue} under Error Prone), so the same resolver drives an Error Prone + * {@code BugChecker}, a raw javac plugin, or a standalone parser-based task unchanged. + * + *

Every resolvable chain is assembled at most once and cached, keyed by the ordered set of sources it declares. If + * any source-adding call in the chain has an argument the resolver cannot reduce to a compile-time constant, the whole + * model is treated as unresolvable and {@link #resolve} returns {@code null} — the caller then abstains rather than + * validating against a partial model. + */ +final class ModelResolver { + + private final Function constantResolver; + private final List importRoots; + private final Map cache = new LinkedHashMap<>(); + + /** + * @param constantResolver maps an expression to its compile-time String value, or {@code null} if it is not a + * compile-time constant (literals and {@code static final String}s included). + * @param importRoots directories to resolve relative {@code addImport("path")} arguments against. + */ + ModelResolver(Function constantResolver, List importRoots) { + this.constantResolver = constantResolver; + this.importRoots = importRoots; + } + + /** + * Attempt to resolve and assemble the model from the expression passed to {@code .model(expr)}. + * + * @param modelExpr the argument expression given to {@code .model(...)}, already dereferenced to its defining + * initializer where possible. + * @return the assembled model, or {@code null} if the chain is not fully statically resolvable. + */ + Model resolve(ExpressionTree modelExpr) { + if (!(modelExpr instanceof MethodInvocationTree)) { + return null; // e.g. a bare identifier we could not dereference, or a method return: abstain. + } + // Collect the assembler chain sources from the outermost call (unwrap/assemble) inward to assembler(). + var sources = new AssemblerSources(); + if (!collectChain((MethodInvocationTree) modelExpr, sources)) { + return null; // encountered a non-resolvable source or an unrecognized chain: abstain. + } + if (!sources.sawAssembler) { + return null; // not actually a Model.assembler() chain. + } + String key = sources.cacheKey(); + Model cached = cache.get(key); + if (cached != null) { + return cached; + } + Model assembled = assemble(sources); + if (assembled != null) { + cache.put(key, assembled); + } + return assembled; + } + + /** Walk a fluent chain of method invocations, gathering statically-known model sources. */ + private boolean collectChain(MethodInvocationTree invocation, AssemblerSources sources) { + if (!(invocation.getMethodSelect() instanceof MemberSelectTree select)) { + return false; + } + String method = select.getIdentifier().toString(); + ExpressionTree receiver = select.getExpression(); + + switch (method) { + case "unwrap", "assemble", "putProperty" -> { + // Structural / no-op-for-us calls; recurse into the receiver. + } + case "discoverModels" -> sources.discoverModels = true; + case "addUnparsedModel" -> { + // addUnparsedModel(String name, String content) + var args = invocation.getArguments(); + if (args.size() != 2) { + return false; + } + String name = constantResolver.apply(args.get(0)); + String content = constantResolver.apply(args.get(1)); + if (name == null || content == null) { + return false; // dynamic content: abstain. + } + sources.unparsed.put(name, content); + } + case "addImport" -> { + // addImport(String path) with a literal path. addImport(URL)/computed paths => abstain. + var args = invocation.getArguments(); + if (args.size() != 1) { + return false; + } + String path = constantResolver.apply(args.get(0)); + if (path == null) { + return false; // e.g. addImport(resourceUrl): abstain. + } + sources.imports.add(path); + } + case "assembler" -> { + // Model.assembler() — the root. Stop here. + sources.sawAssembler = true; + return true; + } + default -> { + // Unknown builder method in the chain: be conservative and abstain. + return false; + } + } + + if (receiver instanceof MethodInvocationTree next) { + return collectChain(next, sources); + } + // Reached the head of the chain without seeing assembler() (e.g. Model.assembler where assembler is a field). + return method.equals("assembler"); + } + + private Model assemble(AssemblerSources sources) { + try { + ModelAssembler assembler = Model.assembler() + .putProperty(ModelAssembler.ALLOW_UNKNOWN_TRAITS, true); + if (sources.discoverModels) { + assembler.discoverModels(); + } + for (var entry : sources.unparsed.entrySet()) { + assembler.addUnparsedModel(entry.getKey(), entry.getValue()); + } + for (String imp : sources.imports) { + Path resolved = resolveImportPath(imp); + if (resolved == null) { + // Could not locate a literal import against any known root: abstain rather than assemble partial. + return null; + } + assembler.addImport(resolved); + } + var result = assembler.assemble(); + if (result.getValidationEvents() + .stream() + .anyMatch(ev -> ev.getSeverity() == Severity.ERROR)) { + // The user's model itself doesn't assemble cleanly; that's their build's job to report, not ours. + return null; + } + return result.unwrap(); + } catch (RuntimeException e) { + // Any assembly failure => we cannot validate; abstain silently. + return null; + } + } + + private Path resolveImportPath(String imp) { + Path direct = Path.of(imp); + if (direct.toFile().exists()) { + return direct; + } + for (String root : importRoots) { + Path candidate = Path.of(root).resolve(imp); + if (candidate.toFile().exists()) { + return candidate; + } + } + return null; + } + + /** Ordered, statically-known sources declared by an assembler chain. */ + private static final class AssemblerSources { + private final Map unparsed = new LinkedHashMap<>(); + private final List imports = new ArrayList<>(); + private boolean discoverModels; + private boolean sawAssembler; + + String cacheKey() { + return "discover=" + discoverModels + ";unparsed=" + unparsed + ";imports=" + imports; + } + } +} diff --git a/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/ResolvedClient.java b/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/ResolvedClient.java new file mode 100644 index 000000000..54ebf4e95 --- /dev/null +++ b/client/dynamic-client-errorprone/src/main/java/software/amazon/smithy/java/dynamicclient/compiler/ResolvedClient.java @@ -0,0 +1,83 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.java.dynamicclient.compiler; + +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.knowledge.TopDownIndex; +import software.amazon.smithy.model.shapes.OperationShape; +import software.amazon.smithy.model.shapes.ServiceShape; +import software.amazon.smithy.model.shapes.ShapeId; + +/** + * The resolution result for one {@code DynamicClient} variable: the model it is built from and the service selected on + * it, plus a precomputed operation-name lookup mirroring {@code DynamicClient}'s own runtime table. + */ +final class ResolvedClient { + + private final Model model; + private final ServiceShape service; + private final Map operations; + + private ResolvedClient(Model model, ServiceShape service, Map operations) { + this.model = model; + this.service = service; + this.operations = operations; + } + + /** + * Build a resolved client from a model and an optional explicit service ID. Mirrors {@code DynamicClient.Builder}: + * an explicit service is used if present; otherwise a single service in the model is auto-detected. If the service + * cannot be determined (zero or multiple, none specified), returns a client with a null model so callers abstain. + */ + static ResolvedClient of(Model model, String serviceIdLiteral) { + if (model == null) { + return new ResolvedClient(null, null, Map.of()); + } + ServiceShape service = selectService(model, serviceIdLiteral); + if (service == null) { + // Same abstain-if-ambiguous stance the runtime takes by throwing; here we simply can't validate. + return new ResolvedClient(null, null, Map.of()); + } + Map ops = new LinkedHashMap<>(); + for (OperationShape op : TopDownIndex.of(model).getContainedOperations(service)) { + ops.put(op.getId().getName(), op); + } + return new ResolvedClient(model, service, ops); + } + + private static ServiceShape selectService(Model model, String serviceIdLiteral) { + if (serviceIdLiteral != null) { + try { + return model.getShape(ShapeId.from(serviceIdLiteral)) + .flatMap(s -> s.asServiceShape()) + .orElse(null); + } catch (RuntimeException e) { + return null; + } + } + var services = model.getServiceShapes(); + return services.size() == 1 ? services.iterator().next() : null; + } + + Model model() { + return model; + } + + ServiceShape service() { + return service; + } + + Map operations() { + return operations; + } + + List sortedOperationNames() { + return operations.keySet().stream().sorted().toList(); + } +} diff --git a/client/dynamic-client-errorprone/src/test/java/software/amazon/smithy/java/dynamicclient/compiler/DynamicClientUsageCheckerTest.java b/client/dynamic-client-errorprone/src/test/java/software/amazon/smithy/java/dynamicclient/compiler/DynamicClientUsageCheckerTest.java new file mode 100644 index 000000000..7fe557b7c --- /dev/null +++ b/client/dynamic-client-errorprone/src/test/java/software/amazon/smithy/java/dynamicclient/compiler/DynamicClientUsageCheckerTest.java @@ -0,0 +1,166 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.java.dynamicclient.compiler; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +/** + * Tests for {@link DynamicClientUsageChecker} using Error Prone's {@link CompilationTestHelper}, which compiles the + * inline sources against the real {@code DynamicClient} with the check active and asserts on the diagnostics via + * {@code // BUG: Diagnostic contains:} markers. This replaces the raw-plugin prototype's shell driver. + */ +class DynamicClientUsageCheckerTest { + + private CompilationTestHelper helper() { + return CompilationTestHelper.newInstance(DynamicClientUsageChecker.class, getClass()); + } + + // A single-line Smithy model embedded as a Java string constant (newlines escaped for the source under test). + private static final String MODEL = String.join( + "\\n", + "$version: \\\"2\\\"", + "namespace smithy.example", + "service Sprockets { operations: [CreateSprocket, GetSprocket] }", + "operation CreateSprocket { input := {} output := {} }", + "operation GetSprocket { input := { id: String } output := { id: String } }"); + + @Test + void flagsUnknownOperation() { + helper() + .addSourceLines( + "Demo.java", + "import java.util.Map;", + "import software.amazon.smithy.java.dynamicclient.DynamicClient;", + "import software.amazon.smithy.model.Model;", + "import software.amazon.smithy.model.shapes.ShapeId;", + "class Demo {", + " static final String MODEL = \"" + MODEL + "\";", + " void run() {", + " DynamicClient client = DynamicClient.builder()", + " .serviceId(ShapeId.from(\"smithy.example#Sprockets\"))", + " .model(Model.assembler().addUnparsedModel(\"demo.smithy\", MODEL).assemble().unwrap())", + " .build();", + " // BUG: Diagnostic contains: Operation 'GetSprokcet' not found", + " client.call(\"GetSprokcet\", Map.of(\"id\", \"1\"));", + " }", + "}") + .doTest(); + } + + @Test + void flagsUnknownInputMember() { + helper() + .addSourceLines( + "Demo.java", + "import java.util.Map;", + "import software.amazon.smithy.java.dynamicclient.DynamicClient;", + "import software.amazon.smithy.model.Model;", + "import software.amazon.smithy.model.shapes.ShapeId;", + "class Demo {", + " static final String MODEL = \"" + MODEL + "\";", + " void run() {", + " DynamicClient client = DynamicClient.builder()", + " .serviceId(ShapeId.from(\"smithy.example#Sprockets\"))", + " .model(Model.assembler().addUnparsedModel(\"demo.smithy\", MODEL).assemble().unwrap())", + " .build();", + " // BUG: Diagnostic contains: 'idd' is not a member of input 'GetSprocketInput'", + " client.call(\"GetSprocket\", Map.of(\"idd\", \"1\"));", + " }", + "}") + .doTest(); + } + + @Test + void acceptsValidCalls() { + helper() + .addSourceLines( + "Demo.java", + "import java.util.Map;", + "import software.amazon.smithy.java.dynamicclient.DynamicClient;", + "import software.amazon.smithy.model.Model;", + "import software.amazon.smithy.model.shapes.ShapeId;", + "class Demo {", + " static final String MODEL = \"" + MODEL + "\";", + " void run() {", + " DynamicClient client = DynamicClient.builder()", + " .serviceId(ShapeId.from(\"smithy.example#Sprockets\"))", + " .model(Model.assembler().addUnparsedModel(\"demo.smithy\", MODEL).assemble().unwrap())", + " .build();", + " client.call(\"GetSprocket\", Map.of(\"id\", \"1\"));", + " client.call(\"CreateSprocket\");", + " }", + "}") + .doTest(); + } + + @Test + void detectsServiceWhenNoServiceIdGiven() { + // Single service in the model: the checker auto-detects it, mirroring the runtime builder. + helper() + .addSourceLines( + "Demo.java", + "import java.util.Map;", + "import software.amazon.smithy.java.dynamicclient.DynamicClient;", + "import software.amazon.smithy.model.Model;", + "class Demo {", + " static final String MODEL = \"" + MODEL + "\";", + " void run() {", + " DynamicClient client = DynamicClient.builder()", + " .model(Model.assembler().addUnparsedModel(\"demo.smithy\", MODEL).assemble().unwrap())", + " .build();", + " // BUG: Diagnostic contains: Operation 'Nope' not found", + " client.call(\"Nope\");", + " }", + "}") + .doTest(); + } + + @Test + void abstainsOnDynamicOperationName() { + // Operation name comes from a parameter: the check must NOT flag it. + helper() + .addSourceLines( + "Demo.java", + "import java.util.Map;", + "import software.amazon.smithy.java.dynamicclient.DynamicClient;", + "import software.amazon.smithy.model.Model;", + "import software.amazon.smithy.model.shapes.ShapeId;", + "class Demo {", + " static final String MODEL = \"" + MODEL + "\";", + " void run(String op) {", + " DynamicClient client = DynamicClient.builder()", + " .serviceId(ShapeId.from(\"smithy.example#Sprockets\"))", + " .model(Model.assembler().addUnparsedModel(\"demo.smithy\", MODEL).assemble().unwrap())", + " .build();", + " client.call(op, Map.of(\"id\", \"1\"));", + " }", + "}") + .doTest(); + } + + @Test + void abstainsWhenModelNotStaticallyResolvable() { + // Model is passed in as a parameter: nothing to resolve, so no flags even for a bogus operation. + helper() + .addSourceLines( + "Demo.java", + "import java.util.Map;", + "import software.amazon.smithy.java.dynamicclient.DynamicClient;", + "import software.amazon.smithy.model.Model;", + "import software.amazon.smithy.model.shapes.ShapeId;", + "class Demo {", + " void run(Model model) {", + " DynamicClient client = DynamicClient.builder()", + " .serviceId(ShapeId.from(\"smithy.example#Sprockets\"))", + " .model(model)", + " .build();", + " client.call(\"TotallyNotAnOperation\", Map.of(\"id\", \"1\"));", + " }", + "}") + .doTest(); + } +} diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 6a6796c60..13b4ade0b 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -29,8 +29,13 @@ jspecify = "1.0.0" commonmark = "0.29.0" jsoup = "1.22.2" aws-api-models = "1.0.269" +errorprone = "2.50.0" [libraries] +errorprone-check-api = { module = "com.google.errorprone:error_prone_check_api", version.ref = "errorprone" } +errorprone-core = { module = "com.google.errorprone:error_prone_core", version.ref = "errorprone" } +errorprone-annotation = { module = "com.google.errorprone:error_prone_annotation", version.ref = "errorprone" } +errorprone-test-helpers = { module = "com.google.errorprone:error_prone_test_helpers", version.ref = "errorprone" } smithy-model = { module = "software.amazon.smithy:smithy-model", version.ref = "smithy" } smithy-codegen = { module = "software.amazon.smithy:smithy-codegen-core", version.ref = "smithy" } smithy-aws-traits = { module = "software.amazon.smithy:smithy-aws-traits", version.ref = "smithy" } diff --git a/settings.gradle.kts b/settings.gradle.kts index 0e57486b3..bf5fe2813 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -53,6 +53,7 @@ include(":client:client-http-boringssl") include(":client:client-rpcv2-cbor") include(":client:client-rpcv2-json") include(":client:dynamic-client") +include(":client:dynamic-client-errorprone") include(":client:client-mock-plugin") include(":client:client-waiters") include(":client:client-rulesengine")