Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix application of @param on child operations #3517

Merged
merged 7 commits into from
Jun 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .chronus/changes/param-override-spread-2024-5-4-20-52-44.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking
changeKind: fix
packages:
- "@typespec/compiler"
---

Fix application of `@param` doc tag on operation create with `op is` to override upstream doc
25 changes: 21 additions & 4 deletions packages/compiler/src/core/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ export function createChecker(program: Program): Checker {
const stdTypes: Partial<StdTypes> = {};
const symbolLinks = new Map<number, SymbolLinks>();
const mergedSymbols = new Map<Sym, Sym>();
const docFromCommentForSym = new Map<Sym, string>();
const augmentDecoratorsForSym = new Map<Sym, AugmentDecoratorStatementNode[]>();
const augmentedSymbolTables = new Map<SymbolTable, SymbolTable>();
const referenceSymCache = new WeakMap<
Expand Down Expand Up @@ -2494,16 +2495,27 @@ export function createChecker(program: Program): Checker {
const name = node.id.sv;
let decorators: DecoratorApplication[] = [];

const parameterModelSym = getOrCreateAugmentedSymbolTable(symbol!.metatypeMembers!).get(
"parameters"
);

if (parameterModelSym?.members) {
const members = getOrCreateAugmentedSymbolTable(parameterModelSym.members);
for (const [name, memberSym] of members) {
const doc = extractParamDoc(node, name);
if (doc) {
docFromCommentForSym.set(memberSym, doc);
}
}
}

// Is this a definition or reference?
let parameters: Model, returnType: Type, sourceOperation: Operation | undefined;
if (node.signature.kind === SyntaxKind.OperationSignatureReference) {
// Attempt to resolve the operation
const baseOperation = checkOperationIs(node, node.signature.baseOperation, mapper);
if (baseOperation) {
sourceOperation = baseOperation;
const parameterModelSym = getOrCreateAugmentedSymbolTable(symbol!.metatypeMembers!).get(
"parameters"
)!;
// Reference the same return type and create the parameters type
const clone = initializeClone(baseOperation.parameters, {
properties: createRekeyableMap(),
Expand All @@ -2512,7 +2524,7 @@ export function createChecker(program: Program): Checker {
clone.properties = createRekeyableMap(
Array.from(baseOperation.parameters.properties.entries()).map(([key, prop]) => [
key,
cloneTypeForSymbol(getMemberSymbol(parameterModelSym, prop.name)!, prop, {
cloneTypeForSymbol(getMemberSymbol(parameterModelSym!, prop.name)!, prop, {
model: clone,
sourceProperty: prop,
}),
Expand Down Expand Up @@ -5574,6 +5586,7 @@ export function createChecker(program: Program): Checker {
if (returnTypesDocs.errors) {
decorators.unshift(createDocFromCommentDecorator("errors", returnTypesDocs.errors));
}
} else if (targetType.kind === "ModelProperty") {
}
return decorators;
}
Expand Down Expand Up @@ -6513,6 +6526,10 @@ export function createChecker(program: Program): Checker {
): T {
let clone = initializeClone(type, additionalProps);
if ("decorators" in clone) {
const docComment = docFromCommentForSym.get(sym);
if (docComment) {
clone.decorators.push(createDocFromCommentDecorator("self", docComment));
}
for (const dec of checkAugmentDecorators(sym, clone, undefined)) {
clone.decorators.push(dec);
}
Expand Down
118 changes: 112 additions & 6 deletions packages/compiler/test/checker/doc-comment.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ import { Model, Operation } from "../../src/core/index.js";
import { getDoc, getErrorsDoc, getReturnsDoc } from "../../src/lib/decorators.js";
import { BasicTestRunner, createTestRunner } from "../../src/testing/index.js";

describe("compiler: checker: doc comments", () => {
let runner: BasicTestRunner;
beforeEach(async () => {
runner = await createTestRunner();
});
let runner: BasicTestRunner;
beforeEach(async () => {
runner = await createTestRunner();
});

describe("compiler: checker: doc comments", () => {
const expectedMainDoc = "This is a doc comment.";
const docComment = `/**
* ${expectedMainDoc}
Expand Down Expand Up @@ -238,8 +238,114 @@ describe("compiler: checker: doc comments", () => {
strictEqual(getErrorsDoc(runner.program, test), "Another string");
});
});
});

describe("@param", () => {
async function getDocForParam(code: string): Promise<string | undefined> {
const { target } = (await runner.compile(code)) as { target: Operation };
ok(target, `Make sure to have @test("target") in code.`);
return getDoc(runner.program, target.parameters.properties.get("one")!);
}

it("applies doc on param", async () => {
const doc = await getDocForParam(`
/**
* @param one Doc comment
*/
@test("target") op base(one: string): void;
`);
strictEqual(doc, "Doc comment");
});

it("@doc on param wins", async () => {
const doc = await getDocForParam(`
/**
* @param one Doc comment
*/
@test("target") op base(@doc("Explicit") one: string): void;
`);
strictEqual(doc, "Explicit");
});

it("augment @@doc on param wins", async () => {
const doc = await getDocForParam(`
/**
* @param one Doc comment
*/
@test("target") op base(one: string): void;

@@doc(base::parameters.one, "Override");
`);
strictEqual(doc, "Override");
});

it("carry over with op is", async () => {
const doc = await getDocForParam(`
/**
* @param one Doc comment
*/
op base(one: string): void;

@test("target") op child is base;
`);
strictEqual(doc, "Doc comment");
});

it("@param on child operation override parent @param", async () => {
const doc = await getDocForParam(`
/**
* @param one Doc comment
*/
op base(one: string): void;

/**
* @param one Override for child
*/
@test("target") op child is base;
`);
strictEqual(doc, "Override for child");
});

it("augment @@doc wins over @param on child operation", async () => {
const doc = await getDocForParam(`
/**
* @param one Doc comment
*/
op base(one: string): void;

/**
* @param one Override for child
*/
@test("target") op child is base;
@@doc(child::parameters.one, "Override for child again");
`);
strictEqual(doc, "Override for child again");
});

it("spread model without @param keeps doc on property", async () => {
const doc = await getDocForParam(`
model A {
@doc("Via model") one: string
}
@test("target") op base(...A): void;
`);
strictEqual(doc, "Via model");
});

it("@param override doc set from spread model", async () => {
const doc = await getDocForParam(`
model A {
@doc("Via model") one: string
}
/**
* @param one Doc comment
*/
@test("target") op base(...A): void;
`);
strictEqual(doc, "Doc comment");
});

it("using @param in doc comment of operation applies doc on the parameters", async () => {
it("applies to distinct parameters", async () => {
// One @param has a hyphen but the other does not (should handle both cases)
const { addUser } = (await runner.compile(`

Expand Down
Loading