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

feat: logic for new config #45

Merged
merged 7 commits into from
Mar 6, 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
9 changes: 8 additions & 1 deletion src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,12 @@ export const defaultFunctions: Functions = {
external: { tags: { dev: false, notice: true, return: true, param: true } },
public: { tags: { dev: false, notice: true, return: true, param: true } },
private: { tags: { dev: false, notice: true, return: true, param: true } },
constructor: false as unknown as Function & boolean,
} as const;

export const defaultTags = {
tags: {
dev: false,
notice: true,
param: true,
},
} as const;
11 changes: 10 additions & 1 deletion src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { getProjectCompiledSources, processConfig } from './utils';
import { Processor } from './processor';
import { Config } from './types';
import { Validator } from './validator';
import { defaultFunctions } from './constants';
import { defaultFunctions, defaultTags } from './constants';

/**
* Main function that processes the sources and prints the warnings
Expand Down Expand Up @@ -76,10 +76,19 @@ async function getConfig(configPath: string): Promise<Config> {
description: 'If set to true, all external and public functions must have @inheritdoc.',
default: true,
},
constructorNatspec: {
type: 'boolean',
description: 'If set to true, all contracts must have a natspec for the constructor.',
default: false,
},
})
.parseSync();

config.functions = defaultFunctions;
config.modifiers = defaultTags;
config.errors = defaultTags;
config.events = defaultTags;
config.structs = defaultTags;

return config as Config;
}
80 changes: 39 additions & 41 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,60 +10,52 @@ import {
import { Static, Type } from '@sinclair/typebox';

// NOTE: For params like `return` if its set to true we will only force it if the function does return something

export const tagSchema = Type.Object({
tags: Type.Object({
dev: Type.Boolean({ default: false }),
notice: Type.Boolean({ default: true }),
param: Type.Boolean({ default: true }),
}),
});

export const functionSchema = Type.Object({
tags: Type.Object({
dev: Type.Boolean({ default: false }),
notice: Type.Boolean({ default: true }),
param: Type.Boolean({ default: true }),
return: Type.Boolean({ default: true }),
}),
});

export const functionConfigSchema = Type.Object({
internal: Type.Optional(
Type.Object({
tags: Type.Object({
dev: Type.Boolean({ default: false }),
notice: Type.Boolean({ default: true }),
return: Type.Boolean({ default: true }),
param: Type.Boolean({ default: true }),
}),
})
),
external: Type.Optional(
Type.Object({
tags: Type.Object({
dev: Type.Boolean({ default: false }),
notice: Type.Boolean({ default: true }),
return: Type.Boolean({ default: true }),
param: Type.Boolean({ default: true }),
}),
})
),
public: Type.Optional(
Type.Object({
tags: Type.Object({
dev: Type.Boolean({ default: false }),
notice: Type.Boolean({ default: true }),
return: Type.Boolean({ default: true }),
param: Type.Boolean({ default: true }),
}),
})
),
private: Type.Optional(
Type.Object({
tags: Type.Object({
dev: Type.Boolean({ default: false }),
notice: Type.Boolean({ default: true }),
return: Type.Boolean({ default: true }),
param: Type.Boolean({ default: true }),
}),
})
),
constructor: Type.Boolean({ default: false }),
internal: functionSchema,

external: functionSchema,

public: functionSchema,

private: functionSchema,
});

export const configSchema = Type.Object({
include: Type.String(),
exclude: Type.String({ default: '' }),
root: Type.String({ default: './' }),
functions: functionConfigSchema,
events: tagSchema,
errors: tagSchema,
modifiers: tagSchema,
structs: tagSchema,
inheritdoc: Type.Boolean({ default: true }),
constructorNatspec: Type.Boolean({ default: false }),
});

export type KeysForSupportedTags = 'events' | 'errors' | 'modifiers' | 'structs';
export type FunctionConfig = Static<typeof functionSchema>;
export type Config = Static<typeof configSchema>;
export type Functions = Static<typeof functionConfigSchema>;
export type Tags = Static<typeof tagSchema>;

export interface NatspecDefinition {
name?: string;
Expand Down Expand Up @@ -103,3 +95,9 @@ export interface IWarning {
location: string;
messages: string[];
}

export type HasVParameters = {
vParameters: {
vParameters: Array<{ name: string }>;
};
};
19 changes: 17 additions & 2 deletions src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import fs from 'fs/promises';
import path from 'path';
import Ajv from 'ajv';
import { Natspec, NatspecDefinition, NodeToProcess, Config, configSchema, Functions } from './types';
import { Natspec, NatspecDefinition, NodeToProcess, Config, configSchema, Functions, KeysForSupportedTags } from './types';
import { ASTKind, ASTReader, SourceUnit, compileSol, FunctionDefinition } from 'solc-typed-ast';
import { defaultFunctions } from './constants';
import { defaultFunctions, defaultTags } from './constants';

/**
* Returns the absolute paths of the Solidity files
Expand Down Expand Up @@ -236,7 +236,12 @@ export async function processConfig(filePath: string): Promise<Config> {
exclude: detectedConfig.exclude ?? '',
root: detectedConfig.root ?? './',
functions: detectedConfig.functions,
events: detectedConfig.events ?? defaultTags,
errors: detectedConfig.errors ?? defaultTags,
modifiers: detectedConfig.modifiers ?? defaultTags,
structs: detectedConfig.structs ?? defaultTags,
inheritdoc: detectedConfig.inheritdoc ?? true,
constructorNatspec: detectedConfig.constructorNatspec ?? false,
};

// Validate the received config matches our expected type
Expand All @@ -250,3 +255,13 @@ export async function processConfig(filePath: string): Promise<Config> {

return config;
}

/**
* Returns if the key being used is for a supported tag
* @dev A "supported tag" is a generalized term for events, errors, modifiers, and structs as they all share the same tag schema
* @param {string} key The key to check
* @returns {boolean} True if the key is for a supported tag
*/
export function isKeyForSupportedTags(key: string): key is KeysForSupportedTags {
return ['events', 'errors', 'modifiers', 'structs'].includes(key);
}
153 changes: 144 additions & 9 deletions src/validator.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
import { Config, Natspec, NodeToProcess } from './types';
import { matchesFunctionKind, getElementFrequency } from './utils';
import {
Config,
FunctionConfig,
Functions,
HasVParameters,
Natspec,
NatspecDefinition,
NodeToProcess,
KeysForSupportedTags,
Tags,
} from './types';
import { matchesFunctionKind, getElementFrequency, isKeyForSupportedTags } from './utils';
import {
EnumDefinition,
ErrorDefinition,
Expand Down Expand Up @@ -50,24 +60,86 @@ export class Validator {
}

// Inheritdoc is not enforced nor present, and there is no other documentation, returning error
if (!natspec.tags.length) return [`Natspec is missing`];
if (!natspec.tags.length) {
let needsWarning = false;
// If node is a function, check the user defined config
if (node instanceof FunctionDefinition) {
Object.keys(this.config.functions).forEach((key) => {
Object.keys(this.config.functions[key as keyof Functions].tags).forEach((tag) => {
if (this.config.functions[key as keyof Functions][tag as keyof FunctionConfig]) {
needsWarning = true;
}
});
Comment on lines +68 to +71
Copy link
Member

@gas1cent gas1cent Mar 5, 2024

Choose a reason for hiding this comment

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

One thing to note here, we could avoid repeating as keyof by converting the config into a class and adding helpers to it. This approach will also help with retrieving the rest of the rules, let's discuss it later when the logic is fully developed.

});
} else {
// The other config rules use the same datatype so we can check them here
Object.keys(this.config).forEach((key) => {
if (isKeyForSupportedTags(key)) {
const tagsConfig = this.config[key]?.tags;
if (tagsConfig) {
Object.values(tagsConfig).forEach((value) => {
if (value) {
needsWarning = true;
}
});
}
}
});
}

if (needsWarning) return [`Natspec is missing`];
}

// Validate the completeness of the documentation
let alerts: string[] = [];
let isDevTagForced: boolean;
let isNoticeTagForced: boolean;

if (node instanceof EnumDefinition) {
// TODO: Process enums
} else if (node instanceof ErrorDefinition) {
alerts = [...alerts, ...this.validateParameters(node, natspecParams)];
isDevTagForced = this.config.errors.tags.dev;
isNoticeTagForced = this.config.errors.tags.notice;

alerts = [
...alerts,
...this.validateParameters(node, natspecParams, 'errors'),
...this.validateTags(isDevTagForced, isNoticeTagForced, natspec.tags),
];
} else if (node instanceof EventDefinition) {
alerts = [...alerts, ...this.validateParameters(node, natspecParams)];
isDevTagForced = this.config.events.tags.dev;
isNoticeTagForced = this.config.events.tags.notice;

alerts = [
...alerts,
...this.validateParameters(node, natspecParams, 'events'),
...this.validateTags(isDevTagForced, isNoticeTagForced, natspec.tags),
];
} else if (node instanceof FunctionDefinition) {
const natspecReturns = natspec.returns.map((p) => p.name);
alerts = [...alerts, ...this.validateParameters(node, natspecParams), ...this.validateReturnParameters(node, natspecReturns)];
isDevTagForced = this.config.functions[node.visibility as keyof Functions]?.tags.dev;
isNoticeTagForced = this.config.functions[node.visibility as keyof Functions]?.tags.notice;

alerts = [
...alerts,
...this.validateParameters(node, natspecParams),
...this.validateReturnParameters(node, natspecReturns),
...this.validateTags(isDevTagForced, isNoticeTagForced, natspec.tags),
];
} else if (node instanceof ModifierDefinition) {
alerts = [...alerts, ...this.validateParameters(node, natspecParams)];
isDevTagForced = this.config.modifiers.tags.dev;
isNoticeTagForced = this.config.modifiers.tags.notice;

alerts = [
...alerts,
...this.validateParameters(node, natspecParams, 'modifiers'),
...this.validateTags(isDevTagForced, isNoticeTagForced, natspec.tags),
];
} else if (node instanceof StructDefinition) {
alerts = [...alerts, ...this.validateMembers(node, natspecParams)];
isDevTagForced = this.config.structs.tags.dev;
isNoticeTagForced = this.config.structs.tags.notice;

alerts = [...alerts, ...this.validateMembers(node, natspecParams), ...this.validateTags(isDevTagForced, isNoticeTagForced, natspec.tags)];
} else if (node instanceof VariableDeclaration) {
// Only the presence of a notice is validated
}
Expand All @@ -82,11 +154,25 @@ export class Validator {
* @param {string[]} natspecParams - The list of parameters from the natspec
* @returns {string[]} - The list of alerts
*/
private validateParameters(node: ErrorDefinition | FunctionDefinition | ModifierDefinition, natspecParams: (string | undefined)[]): string[] {
private validateParameters<T extends HasVParameters>(
node: T,
natspecParams: (string | undefined)[],
key: KeysForSupportedTags | undefined = undefined
): string[] {
let definedParameters = node.vParameters.vParameters.map((p) => p.name);
let alerts: string[] = [];
const counter = getElementFrequency(natspecParams);

if (node instanceof FunctionDefinition) {
if (!this.config.functions[node.visibility as keyof Functions]?.tags.param) {
return [];
}
} else if (key !== undefined) {
if (!this.config[key]?.tags.param) {
return [];
}
}

for (let paramName of definedParameters) {
if (!natspecParams.includes(paramName)) {
alerts.push(`@param ${paramName} is missing`);
Expand All @@ -105,6 +191,10 @@ export class Validator {
* @returns {string[]} - The list of alerts
*/
private validateMembers(node: StructDefinition, natspecParams: (string | undefined)[]): string[] {
if (!this.config.structs.tags.param) {
return [];
}

let members = node.vMembers.map((p) => p.name);
let alerts: string[] = [];
const counter = getElementFrequency(natspecParams);
Expand All @@ -128,6 +218,11 @@ export class Validator {
* @returns {string[]} - The list of alerts
*/
private validateReturnParameters(node: FunctionDefinition, natspecReturns: (string | undefined)[]): string[] {
// If return tags are not enforced, return no warnings
if (!this.config.functions[node.visibility as keyof Functions]?.tags.return) {
return [];
}

let alerts: string[] = [];
let functionReturns = node.vReturnParameters.vParameters.map((p) => p.name);

Expand All @@ -145,6 +240,46 @@ export class Validator {
return alerts;
}

private validateTags(isDevTagForced: boolean, isNoticeTagForced: boolean, natspecTags: NatspecDefinition[]): string[] {
// If both are disabled no warnings should emit so we dont need to check anything
if (!isDevTagForced && !isNoticeTagForced) {
return [];
}

let alerts: string[] = [];

let devCounter = 0;
let noticeCounter = 0;

for (const tag of natspecTags) {
if (tag.name === 'dev') {
devCounter++;
} else if (tag.name === 'notice') {
noticeCounter++;
}
}

// Needs a dev tag
// More then one dev tag is ok
if (isDevTagForced && devCounter === 0) {
alerts.push(`@dev is missing`);
}

if (isNoticeTagForced) {
// Needs one notice tag
if (noticeCounter === 0) {
alerts.push(`@notice is missing`);
}

// Cant have more then one notice tag
if (noticeCounter > 1) {
alerts.push(`@notice is duplicated`);
}
}
Comment on lines +274 to +277
Copy link
Member

Choose a reason for hiding this comment

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

I believe it's a valid scenario. At the same time, having duplicated @param tags should be prohibited.


return alerts;
}

/**
* Checks if the node requires inheritdoc
* @param {NodeToProcess} node - The node to process
Expand Down
Loading
Loading