Skip to content
Open
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
11 changes: 9 additions & 2 deletions Extension/src/LanguageServer/Providers/codeActionProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ export class CodeActionProvider implements vscode.CodeActionProvider {
const disableCodeActions: vscode.CodeAction[] = [];
const removeCodeActions: vscode.CodeAction[] = [];
const docCodeActions: vscode.CodeAction[] = [];
const docTargets: Set<string> = new Set<string>();
const showClear: string = new CppSettings().clangTidyCodeActionShowClear;

// Check which code actions to show. This can get called a lot
Expand Down Expand Up @@ -174,10 +175,16 @@ export class CodeActionProvider implements vscode.CodeActionProvider {
}
}

if (codeActionCodeInfo === undefined || codeActionCodeInfo.docCodeAction === undefined) {
if (codeAction.docCodeAction === undefined) {
continue;
}
docCodeActions.push(codeActionCodeInfo.docCodeAction);
// Same check code can show up for several folders, and the target depends on
// the clang-tidy release each folder runs, so keep the URLs distinct.
const docTarget: string | undefined = codeAction.docCodeAction.command?.arguments?.[0]?.toString();
if (docTarget !== undefined && !docTargets.has(docTarget)) {
docTargets.add(docTarget);
docCodeActions.push(codeAction.docCodeAction);
}
}
if (fixCodeActions.length > 0) {
resultCodeActions.push(...fixCodeActions);
Expand Down
32 changes: 32 additions & 0 deletions Extension/src/LanguageServer/clangTidyDocumentation.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/* --------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All Rights Reserved.
* See 'LICENSE' in the project root for license information.
* ------------------------------------------------------------------------------------------ */

/**
* The clang-tidy documentation is published per major LLVM release, so a check's page lives
* under the .1.0 release of the major the binary in use comes from: 23.1.0 and 22.1.0 have
* pages, 23.1.1 and 22.1.3 do not. Checks added in a newer major have no page in an older one,
* so the URL follows the binary in use rather than the release bundled with the extension.
* When the version in use could not be determined, fall back to the bundled release.
*/
const versionPattern: RegExp = /^([0-9]+)\.[0-9]+\.[0-9]+$/;

/** The release a clang-tidy version's documentation was published under. */
function documentationRelease(clangTidyVersion: string): string | undefined {
const match: RegExpMatchArray | null = clangTidyVersion.match(versionPattern);
return match ? `${match[1]}.1.0` : undefined;
}

/**
* The clang-tidy release bundled with the extension. Its documentation is published under the
* .1.0 release of its major, so the fallback follows this rather than spelling out a URL version.
*/
export const bundledClangTidyVersion: string = '23.1.1';

export const fallbackClangTidyDocVersion: string = documentationRelease(bundledClangTidyVersion) ?? '23.1.0';

export function getClangTidyDocumentationUrl(docPage: string, clangTidyVersion?: string): string {
const release: string | undefined = clangTidyVersion ? documentationRelease(clangTidyVersion) : undefined;
return `https://releases.llvm.org/${release ?? fallbackClangTidyDocVersion}/tools/clang/tools/extra/docs/clang-tidy/${docPage}`;
}
33 changes: 16 additions & 17 deletions Extension/src/LanguageServer/codeAnalysis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import * as vscode from 'vscode';
import { NotificationType, Range } from 'vscode-languageclient';
import * as nls from 'vscode-nls';
import { getClangTidyDocumentationUrl } from './clangTidyDocumentation';
import { Location, WorkspaceEdit } from './commonTypes';
import { CppSourceStr } from './extension';
import { LanguageClient } from './languageClient';
Expand Down Expand Up @@ -34,6 +35,7 @@ export interface CodeActionDiagnosticInfo {
fixCodeAction?: vscode.CodeAction;
// suppressCodeAction?: vscode.CodeAction; // TODO?
removeCodeAction?: vscode.CodeAction;
docCodeAction?: vscode.CodeAction;
}

// Used to handle fix invalidation after an edit,
Expand Down Expand Up @@ -63,7 +65,6 @@ export interface CodeActionCodeInfo {
fixAllTypeCodeAction?: vscode.CodeAction;
disableAllTypeCodeAction?: vscode.CodeAction;
removeAllTypeCodeAction?: vscode.CodeAction;
docCodeAction?: vscode.CodeAction;
}

interface CodeActionAllInfo {
Expand Down Expand Up @@ -379,24 +380,22 @@ export function publishCodeAnalysisDiagnostics(params: PublishCodeAnalysisDiagno
const checksPage: string = dashIndex > 0 ? primaryCode.substring(dashIndex + 1) : primaryCode;
docPage = `checks${checksGroup}/${checksPage}.html`;
}
// TODO: This should be checking the clang-tidy version used to better support usage of older versions.
const primaryDocUri: vscode.Uri = vscode.Uri.parse(`https://releases.llvm.org/23.1.0/tools/clang/tools/extra/docs/clang-tidy/${docPage}`);
const primaryDocUri: vscode.Uri = vscode.Uri.parse(getClangTidyDocumentationUrl(docPage, new CppSettings(realUri).clangTidyVersion));

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You are right, and this one was mine. The action was stored on the code, so the first folder to report a code decided the release for every folder and the URL never moved when the path or version changed. It now lives on the diagnostic, which already knows the resource, and the provider keeps one action per target URL so a shared code does not produce duplicates.

diagnostic.code = { value: identifier.code, target: primaryDocUri };

if (new CppSettings().clangTidyCodeActionShowDocumentation) {
if (codeActionCodeInfo.docCodeAction === undefined) {
codeActionCodeInfo.docCodeAction = {
title: localize("show.documentation.for", "Show documentation for {0}", primaryCode),
command: {
title: 'ShowDocumentation',
command: 'C_Cpp.ShowCodeAnalysisDocumentation',
arguments: [primaryDocUri]
},
kind: vscode.CodeActionKind.QuickFix
};
}
} else {
codeActionCodeInfo.docCodeAction = undefined;
// The target URL follows the clang-tidy version in use for this resource, so the
// action belongs to the diagnostic. Caching it per check code instead would keep
// whichever folder reported first and reuse its release everywhere.
if (new CppSettings(realUri).clangTidyCodeActionShowDocumentation) {
codeAction.docCodeAction = {
title: localize("show.documentation.for", "Show documentation for {0}", primaryCode),
command: {
title: 'ShowDocumentation',
command: 'C_Cpp.ShowCodeAnalysisDocumentation',
arguments: [primaryDocUri]
},
kind: vscode.CodeActionKind.QuickFix
};
}
} else {
diagnostic.code = d.code;
Expand Down
19 changes: 18 additions & 1 deletion Extension/src/LanguageServer/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import * as vscode from 'vscode';
import * as nls from 'vscode-nls';
import * as which from 'which';
import * as util from '../common';
import { getCachedClangFormatPath, getCachedClangTidyPath, getExtensionFilePath, getRawSetting, isArray, isArrayOfString, isBoolean, isNumber, isObject, isString, isValidMapping, setCachedClangFormatPath, setCachedClangTidyPath } from '../common';
import { getCachedClangFormatPath, getCachedClangTidyPath, getCachedClangTidyVersion, getExtensionFilePath, getRawSetting, isArray, isArrayOfString, isBoolean, isNumber, isObject, isString, isValidMapping, setCachedClangFormatPath, setCachedClangTidyPath, setCachedClangTidyVersion } from '../common';
import { isWindows } from '../constants';
import * as telemetry from '../telemetry';
import { cachedEditorConfigLookups, DefaultClient, hasTrustedCompilerPaths } from './client';
Expand Down Expand Up @@ -279,6 +279,14 @@ export class CppSettings extends Settings {
return this.getClangPath(false);
}

public get clangTidyVersion(): string | undefined {
// The cached version comes from an auto-discovered binary, so it says nothing about the
// clang-tidy a resource carrying its own path is running. Reporting it would point that
// resource's documentation links at the wrong release, so leave the version unknown and
// let the caller fall back.
return changeBlankStringToUndefined(this.getAsStringOrUndefined("codeAnalysis.clangTidy.path")) ? undefined : getCachedClangTidyVersion();
}

public get clangFormatPath(): string | undefined {
return this.getClangPath(true);
}
Expand All @@ -303,6 +311,7 @@ export class CppSettings extends Settings {
} else {
// Attempt to invoke both our own version of clang-* to see if we can successfully execute it, and to get its version.
let bundledVersion: string;
let clangVersion: string | undefined;
try {
const bundledPath: string = getExtensionFilePath(`./LLVM/bin/${clangName}`);
const output: string = execFileSync(bundledPath, ['--version']).toString();
Expand All @@ -322,10 +331,18 @@ export class CppSettings extends Settings {
if (semver.ltr(userVersion, bundledVersion)) {
path = "";
setCachedClangPath(path);
clangVersion = bundledVersion;
} else {
clangVersion = userVersion;
}
} catch {
path = "";
setCachedClangPath(path);
clangVersion = bundledVersion;
}

if (!isFormat && clangVersion) {
setCachedClangTidyVersion(clangVersion);
}
}
}
Expand Down
9 changes: 9 additions & 0 deletions Extension/src/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,15 @@ export function setCachedClangTidyPath(path: string): void {
cachedClangTidyPath = path;
}

let cachedClangTidyVersion: string | undefined;
export function getCachedClangTidyVersion(): string | undefined {
return cachedClangTidyVersion;
}

export function setCachedClangTidyVersion(version: string): void {
cachedClangTidyVersion = version;
}

// Use this package.json to read values
export const packageJson: any = vscode.extensions.getExtension("ms-vscode.cpptools")?.packageJSON;

Expand Down
41 changes: 41 additions & 0 deletions Extension/test/unit/clangTidyDocumentation.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/* --------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All Rights Reserved.
* See 'LICENSE' in the project root for license information.
* ------------------------------------------------------------------------------------------ */

import { describe, it } from 'mocha';
import { strictEqual } from 'node:assert';
import { bundledClangTidyVersion, fallbackClangTidyDocVersion, getClangTidyDocumentationUrl } from '../../src/LanguageServer/clangTidyDocumentation';

const bundledDocUrl: string = `https://releases.llvm.org/${fallbackClangTidyDocVersion}/tools/clang/tools/extra/docs/clang-tidy`;

describe('clang-tidy documentation URLs', () => {
it('points at the clang-tidy version in use', () => {
strictEqual(
getClangTidyDocumentationUrl('checks/readability/use-std-min-max.html', '19.1.0'),
'https://releases.llvm.org/19.1.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability/use-std-min-max.html');
});

it('uses the .1.0 release of that major, since patch releases have no page', () => {
strictEqual(
getClangTidyDocumentationUrl('checks/readability/use-std-min-max.html', '23.1.1'),
'https://releases.llvm.org/23.1.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability/use-std-min-max.html');
strictEqual(
getClangTidyDocumentationUrl('checks/misc/include-cleaner.html', '22.1.3'),
'https://releases.llvm.org/22.1.0/tools/clang/tools/extra/docs/clang-tidy/checks/misc/include-cleaner.html');
});

it('falls back to the bundled version when the version in use is unknown', () => {
strictEqual(getClangTidyDocumentationUrl('checks/misc/include-cleaner.html'), `${bundledDocUrl}/checks/misc/include-cleaner.html`);
strictEqual(getClangTidyDocumentationUrl('checks/misc/include-cleaner.html', ''), `${bundledDocUrl}/checks/misc/include-cleaner.html`);
strictEqual(getClangTidyDocumentationUrl('checks/misc/include-cleaner.html', '23.1'), `${bundledDocUrl}/checks/misc/include-cleaner.html`);
});

it('keeps the fallback on the release the bundled binary belongs to', () => {
// The fallback is only correct while it matches what the bundled clang-tidy would produce,
// which is the point of deriving it from the bundled version instead of a literal URL.
strictEqual(
getClangTidyDocumentationUrl('checks/misc/include-cleaner.html', bundledClangTidyVersion),
getClangTidyDocumentationUrl('checks/misc/include-cleaner.html'));
});
});