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.

Sign up for GitHub

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

feat(generate:assertTags): Enable per package assert tagging configuration #23452

Merged
merged 23 commits into from
Jan 21, 2025
Merged

feat(generate:assertTags): Enable per package assert tagging configuration #23452

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
aadc2e0
Setup tagging for tree's "fail"
CraigMacomber Jan 3, 2025
c16f987
Fix fail tagging
CraigMacomber Jan 3, 2025
5e1ce1f
remove fail templates
CraigMacomber Jan 3, 2025
89ce236
Update fail to allow tagged messages
CraigMacomber Jan 3, 2025
3d5fee3
Merge branch 'main' into failTag
CraigMacomber Jan 7, 2025
5c06f20
Work on comments
CraigMacomber Jan 7, 2025
e55dd41
fix error aggregation and extend comment
CraigMacomber Jan 7, 2025
a67474c
Apply suggestions from code review
CraigMacomber Jan 7, 2025
2c8fe5c
workaround assert tagging to make fail call assert
CraigMacomber Jan 7, 2025
137a6c2
Fix newAssetFiles typo
CraigMacomber Jan 7, 2025
472cf03
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
CraigMacomber Jan 8, 2025
fbac2c2
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
CraigMacomber Jan 8, 2025
18ae12c
Give assert tagging its own config
CraigMacomber Jan 8, 2025
c400e3c
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
CraigMacomber Jan 9, 2025
46cfaa3
remove old config
CraigMacomber Jan 9, 2025
23cc67d
Merge branch 'main' into failTag
CraigMacomber Jan 13, 2025
5a7b3dc
Document config
CraigMacomber Jan 14, 2025
b2fca3d
Document configs
CraigMacomber Jan 14, 2025
9593d26
Update generated docs
CraigMacomber Jan 14, 2025
24efa74
Merge branch 'main' into failTag
CraigMacomber Jan 16, 2025
01d79dc
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
CraigMacomber Jan 16, 2025
240b8ab
Restrict config to only config.mjs
CraigMacomber Jan 16, 2025
3d42046
Merge branch 'main' into failTag
CraigMacomber Jan 21, 2025
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
5 changes: 2 additions & 3 deletions build-tools/packages/build-cli/api-report/build-cli.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,9 @@ import { InterdependencyRange } from '@fluid-tools/version-tools';
import { run } from '@oclif/core';
import { VersionBumpType } from '@fluid-tools/version-tools';

// @public (undocumented)
// @public
export interface AssertTaggingConfig {
// (undocumented)
assertionFunctions: {
assertionFunctions?: {
[functionName: string]: number;
};
enabledPaths?: RegExp[];
Expand Down
228 changes: 144 additions & 84 deletions build-tools/packages/build-cli/src/commands/generate/assertTags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@
* Licensed under the MIT License.
*/

import { strict as assert } from "node:assert";
import fs from "node:fs";
import path from "node:path";
import { Package } from "@fluidframework/build-tools";
import { PackageCommand } from "../../BasePackageCommand.js";
import { PackageKind } from "../../filter.js";
import { PackageKind, type PackageWithKind } from "../../filter.js";

import { Flags } from "@oclif/core";
import {
Expand All @@ -20,13 +19,35 @@ import {
StringLiteral,
SyntaxKind,
} from "ts-morph";
import { getFlubConfig } from "../../config.js";

const shortCodes = new Map<number, Node>();
const newAssetFiles = new Set<SourceFile>();
const codeToMsgMap = new Map<string, string>();
let maxShortCode = -1;
/**
* Key is the name of the assert function.
* Value is the index of the augment to tag.
* @remarks
* The function names are not handled in a scoped/qualified way, so any function imported or declared with that name will have tagging applied.
* See also {@link AssertTaggingConfig.assertionFunctions}.
*/
type AssertionFunctions = ReadonlyMap<string, number>;

const defaultAssertionFunctions: ReadonlyMap<string, number> = new Map([["assert", 1]]);
const defaultAssertionFunctions: AssertionFunctions = new Map([["assert", 1]]);

/**
* Data aggregated about a collection of packages.
*/
interface CollectedData {
readonly shortCodes: Map<number, Node>;
readonly codeToMsgMap: Map<string, string>;
maxShortCode: number;
}

/**
* Data about a specific package.
*/
interface PackageData {
readonly newAssetFiles: ReadonlySet<SourceFile>;
readonly assertionFunctions: AssertionFunctions;
}

export class TagAssertsCommand extends PackageCommand<typeof TagAssertsCommand> {
static readonly summary =
Expand All @@ -47,10 +68,35 @@ export class TagAssertsCommand extends PackageCommand<typeof TagAssertsCommand>

protected defaultSelection = undefined;

private assertionFunctions: ReadonlyMap<string, number> | undefined;
private readonly errors: string[] = [];

protected async selectAndFilterPackages(): Promise<void> {
// TODO:
// Refactor regex based filtering logic.
// Consider doing one the below instead of applying this filtering here:
// a. Add regex filter CLI option to PackageCommand.
// b. Have packages which want to opt out do so in their own configuration.
// c. Refactor TagAssertsCommand so it doesn't have to rely on this override and undocumented implementation details to do this filtering
// (Ex: move the logic into an early continue in processPackages)
// d. Refactor PackageCommand so having subclasses customize filtering is well supported
// (ex: allow the subclass to provide a filtering predicate, perhaps via the constructor or an a method explicitly documented to be used for overriding which normally just returns true).
//
// The current approach isn't ideal from a readability or maintainability perspective.
// 1. selectAndFilterPackages is undocumented had relies on side effects.
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
// To override it correctly the the subclass must know and depend on many undocumented details of the base class (like that this method sets filteredPackages, that its ok for it to modify filteredPackages).
// This makes the base class fragile: refactoring it to work slightly differently (like cache data derived from the set of filteredPackages after they are computed) could break things.
// 2. Data flow is hard to follow. This method does not have inputs or outputs declared in its signature, and the values it reads from the class arn't readonly so its hard to know what is initialized when
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
// and which values are functions of which other values.
// 3. The division of responsibility here is odd. Generally the user of a PackageCommand selects which packages to apply it to on the command line.
// Currently this is done by passing --all (as specified in the script that invokes this command),
// and a separate regex in a config.
// This extra config even more confusing since typically this kind of configuration is per package,
// but in this case the config from one package is applying to multiple release groups based on the working directory the command is run in.
// Normally configuration in a package applies to the package, and sometimes configuration for a release group lives at its root.
// In this case configuration spanning multiple release groups lives in the root of one of them but uses the same file we would use for per package configuration which makes it hard to understand.
Comment on lines +114 to +120
Copy link
Member

Choose a reason for hiding this comment

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

Great observation. This is a side effect of the evolution of the code and the developers writing it. :)

@zhenmichael Let's chat about the specific ideas Craig shared and integrate them into the design for PackageCommand vnext. The most critical thing to decide in order to unblock this PR is where the per-package config should live (see other comments). Any enhancements to the selection/filtering should live in build-infrastructure with the new functions there. Some of the things called out here will likely be addressed as a side effect of building on the new selection/filtering stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If PackageCommand allowed passing both --releaseGroup and --package at the same time then we could use that to replace the regex filter. Thats likely the simplest solution since we could just pass in --releaseGroup client --package @fluidframework/common-utils instead of --all.

If we do want to keep the regex functionality, it can be kept and moved to PackageCommand along with its other filtering options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Supporting multiple selection flags seems trivial, so I'm going to try and make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should do it: #23511

// It seems like it would be more consistent with the design, and more useful generally, that if additional package filtering functionality is needed (ex: regex based filtering) that it
// be added to PackageCommand's package filtering CLI options so all PackageCommands could use it.
// This would remove the need to expose so many internals (like this method, filteredPackages, etc) of PackageCommand as "protected"
// improved testability (since this logic could reside in the more testable selectAndFilterPackages free function) and keep the per package configuration files actually per package
// while putting the cross release group configuration (--all and the regex) in the same place.
protected override async selectAndFilterPackages(): Promise<void> {
await super.selectAndFilterPackages();

const context = await this.getContext();
Expand All @@ -59,11 +105,6 @@ export class TagAssertsCommand extends PackageCommand<typeof TagAssertsCommand>
? undefined
: assertTagging?.enabledPaths;

this.assertionFunctions =
assertTagging?.assertionFunctions === undefined
? defaultAssertionFunctions
: new Map<string, number>(Object.entries(assertTagging.assertionFunctions));

// Further filter packages based on the path regex
const before = this.filteredPackages?.length ?? 0;
this.filteredPackages = this.filteredPackages?.filter((pkg) => {
Expand Down Expand Up @@ -97,71 +138,106 @@ export class TagAssertsCommand extends PackageCommand<typeof TagAssertsCommand>
}
}

protected async processPackage<TPkg extends Package>(
// This should not be called due to processPackages being overridden instead.
protected override async processPackage<TPkg extends Package>(
pkg: TPkg,
kind: PackageKind,
): Promise<void> {
const tsconfigPath = await this.getTsConfigPath(pkg);
this.collectAssertData(tsconfigPath);
throw new Error("Method not implemented.");
}

public async run(): Promise<void> {
// Calls processPackage on all packages to collect assert data.
await super.run();
protected override async processPackages(packages: PackageWithKind[]): Promise<string[]> {
const errors: string[] = [];

const collected: CollectedData = {
shortCodes: new Map<number, Node>(),
codeToMsgMap: new Map<string, string>(),
maxShortCode: -1,
};

const dataMap = new Map<PackageWithKind, PackageData>();

for (const pkg of packages) {
// Package configuration:
// eslint-disable-next-line no-await-in-loop
const tsconfigPath = await this.getTsConfigPath(pkg);
const packageConfig = getFlubConfig(pkg.directory).assertTagging;
Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I see you're loading the package config here. I think putting it in package.json will be clearer and lets you get rid of some of this code since you can just get the config from the package directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting to:

  1. not use FlubConfig for this, or
  2. to put it in the package.json as part of the FlubConfig?

I think you mean 1, which makes sense, though I suspect it won't be less code just less over all coupling.

Copy link
Member

Choose a reason for hiding this comment

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

I'm suggesting putting the settings in package.json, but not with the existing FlubConfig interface. Maybe it makes sense to use the AssertTaggingConfig interface? I was imagining something like this in the individual packages:

{
  "flubCommandConfig":
    "generate:asserts": {
      "assertionFunctions": {
        "assert": 1,
        "fail": 0,
	}
}

We could get rid of the "flubCommandConfig" layer, and maybe that's a bad name anyway. I was trying to avoid adding a bunch of stuff at the top level, but I'm more pragmatic than principled in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping the ability to have one config file cover all 13 packages in packages/test is useful, so I kept the use of cosmic config, but make it use its own config. THis should prevent this folder config from causing issues with any other configurations.

const assertionFunctions: AssertionFunctions =
packageConfig?.assertionFunctions === undefined
? defaultAssertionFunctions
: new Map<string, number>(Object.entries(packageConfig.assertionFunctions));

// load the project based on the tsconfig
const project = new Project({
skipFileDependencyResolution: true,
tsConfigFilePath: tsconfigPath,
});

const newAssetFiles = this.collectAssertData(
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
project,
assertionFunctions,
collected,
errors,
);
dataMap.set(pkg, { assertionFunctions, newAssetFiles });
}

// Tag asserts based on earlier collected data.
this.tagAsserts(true);
}
if (errors.length > 0) {
return errors;
}

private collectAssertData(tsconfigPath: string): void {
// TODO: this can probably be removed now
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this caused many more packages to be processed, like test-dds-utils, test-runtime-utils and everything in packages/test . See the config files added to filter them out.

if (tsconfigPath.includes("test")) {
return;
for (const [pkg, data] of dataMap) {
errors.push(...this.tagAsserts(collected, data));
}

// load the project based on the tsconfig
const project = new Project({
skipFileDependencyResolution: true,
tsConfigFilePath: tsconfigPath,
});
writeShortCodeMappingFile(collected.codeToMsgMap);

return errors;
}

private collectAssertData(
project: Project,
assertionFunctions: AssertionFunctions,
collected: CollectedData,
errors: string[],
): Set<SourceFile> {
const templateErrors: Node[] = [];
const otherErrors: Node[] = [];
const newAssetFiles = new Set<SourceFile>();
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved

// walk all the files in the project
for (const sourceFile of project.getSourceFiles()) {
// walk the assert message params in the file
assert(this.assertionFunctions !== undefined, "No assert functions are defined!");
for (const msg of getAssertMessageParams(sourceFile, this.assertionFunctions)) {
for (const msg of getAssertMessageParams(sourceFile, assertionFunctions)) {
const nodeKind = msg.getKind();
switch (nodeKind) {
// If it's a number, validate it's a shortcode
case SyntaxKind.NumericLiteral: {
const numLit = msg as NumericLiteral;
if (!numLit.getText().startsWith("0x")) {
this.errors.push(
errors.push(
`Shortcodes must be provided by automation and be in hex format: ${numLit.getText()}\n\t${getCallsiteString(
numLit,
)}`,
);
return;
return newAssetFiles;
}
const numLitValue = numLit.getLiteralValue();
if (shortCodes.has(numLitValue)) {
if (collected.shortCodes.has(numLitValue)) {
// if we find two usages of the same short code then fail
this.errors.push(
errors.push(
`Duplicate shortcode 0x${numLitValue.toString(
16,
)} detected\n\t${getCallsiteString(
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
shortCodes.get(numLitValue)!,
collected.shortCodes.get(numLitValue)!,
)}\n\t${getCallsiteString(numLit)}`,
);
return;
return newAssetFiles;
}
shortCodes.set(numLitValue, numLit);
collected.shortCodes.set(numLitValue, numLit);
// calculate the maximun short code to ensure we don't duplicate
maxShortCode = Math.max(numLitValue, maxShortCode);
collected.maxShortCode = Math.max(numLitValue, collected.maxShortCode);

// If comment already exists, extract it for the mapping file
const comments = msg.getTrailingCommentRanges();
Expand Down Expand Up @@ -191,7 +267,7 @@ export class TagAssertsCommand extends PackageCommand<typeof TagAssertsCommand>
originalErrorText.length - 1,
);
}
codeToMsgMap.set(numLit.getText(), originalErrorText);
collected.codeToMsgMap.set(numLit.getText(), originalErrorText);
}
break;
}
Expand Down Expand Up @@ -239,10 +315,16 @@ export class TagAssertsCommand extends PackageCommand<typeof TagAssertsCommand>
if (errorMessages.length > 0) {
this.error(errorMessages.join("\n\n"), { exit: 1 });
}

return newAssetFiles;
}

// TODO: the resolve = true may be safe to remove since we always want to resolve when running this command
private tagAsserts(resolve: true): void {
/**
* Updates source files, adding new asserts to `collected`.
*
* @returns array of error strings.
*/
private tagAsserts(collected: CollectedData, packageData: PackageData): string[] {
const errors: string[] = [];

// eslint-disable-next-line unicorn/consistent-function-scoping
Expand All @@ -256,44 +338,28 @@ export class TagAssertsCommand extends PackageCommand<typeof TagAssertsCommand>
}

// go through all the newly collected asserts and add short codes
for (const s of newAssetFiles) {
for (const s of packageData.newAssetFiles) {
// another policy may have changed the file, so reload it
s.refreshFromFileSystemSync();
assert(this.assertionFunctions !== undefined, "No assert functions are defined!");
for (const msg of getAssertMessageParams(s, this.assertionFunctions)) {
for (const msg of getAssertMessageParams(s, packageData.assertionFunctions)) {
// here we only want to look at those messages that are strings,
// as we validated existing short codes above
if (isStringLiteral(msg)) {
// resolve === fix
if (resolve) {
// for now we don't care about filling gaps, but possible
const shortCode = ++maxShortCode;
shortCodes.set(shortCode, msg);
const text = msg.getLiteralText();
const shortCodeStr = `0x${shortCode.toString(16).padStart(3, "0")}`;
// replace the message with shortcode, and put the message in a comment
msg.replaceWithText(`${shortCodeStr} /* ${text} */`);
codeToMsgMap.set(shortCodeStr, text);
} else {
// TODO: if we are not in resolve mode we
// allow messages that are not short code. this seems like the right
// behavior for main. we may want to enforce shortcodes in release branches in the future
// errors.push(`no assert shortcode: ${getCallsiteString(msg)}`);
break;
}
// for now we don't care about filling gaps, but possible
const shortCode = ++collected.maxShortCode;
collected.shortCodes.set(shortCode, msg);
const text = msg.getLiteralText();
const shortCodeStr = `0x${shortCode.toString(16).padStart(3, "0")}`;
// replace the message with shortcode, and put the message in a comment
msg.replaceWithText(`${shortCodeStr} /* ${text} */`);
collected.codeToMsgMap.set(shortCodeStr, text);
}
}
if (resolve) {
s.saveSync();
}
}

if (resolve) {
writeShortCodeMappingFile();
}
if (errors.length > 0) {
this.error(errors.join("\n"), { exit: 1 });
s.saveSync();
}

return errors;
}

private async getTsConfigPath(pkg: Package): Promise<string> {
Expand All @@ -310,13 +376,6 @@ function getCallsiteString(msg: Node): string {
return `${msg.getSourceFile().getFilePath()}:${msg.getStartLineNumber()}`;
}

/**
* Map from assertion function name to the index of its message argument.
*
* TODO:
* This should be moved into a configuration file.
*/

/**
* Given a source file, this function will look for all assert functions contained in it and return the message parameters.
* This includes both functions named "assert" and ones named "fail"
Expand All @@ -343,7 +402,7 @@ function getAssertMessageParams(
return messageArgs;
}

function writeShortCodeMappingFile(): void {
function writeShortCodeMappingFile(codeToMsgMap: Map<string, string>): void {
// eslint-disable-next-line unicorn/prefer-spread, @typescript-eslint/no-unsafe-assignment
const mapContents = Array.from(codeToMsgMap.entries())
.sort()
Expand All @@ -355,6 +414,7 @@ function writeShortCodeMappingFile(): void {
return accum;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
}, {} as any);
// TODO: this should prabably come from configuration (if each package can have their own) or a CLI argument.
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
const targetFolder = "packages/runtime/test-runtime-utils/src";

if (!fs.existsSync(targetFolder)) {
Expand Down
Loading
Loading