Skip to content

Remove uses of property existence with in #6932

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

Merged
alexr00 merged 3 commits into from
Aug 11, 2025
Merged

Remove uses of property existence with in #6932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28cf23a
Remove uses of property existence with `in`
alexr00 May 8, 2025
7eb1133
Merge branch 'main' into alexr00/particular-bug
alexr00 May 9, 2025
4f91735
Merge branch 'main' into alexr00/particular-bug
alexr00 Aug 11, 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
20 changes: 10 additions & 10 deletions resources/icons/dark/output.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
16 changes: 12 additions & 4 deletions src/commands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1331,12 +1331,20 @@ ${contents}
}),
);

context.subscriptions.push(vscode.commands.registerCommand('review.createSuggestionsFromChanges', async (value: ({ resourceStates: { resourceUri }[] }) | ({ resourceUri: vscode.Uri }), ...additionalSelected: ({ resourceUri: vscode.Uri })[]) => {
interface SCMResourceStates {
resourceStates: { resourceUri: vscode.Uri }[];
}
interface SCMResourceUri {
resourceUri: vscode.Uri;
}
context.subscriptions.push(vscode.commands.registerCommand('review.createSuggestionsFromChanges', async (value: SCMResourceStates | SCMResourceUri, ...additionalSelected: SCMResourceUri[]) => {
let resources: vscode.Uri[];
if ('resourceStates' in value) {
resources = value.resourceStates.map(resource => resource.resourceUri);
const asResourceStates = value as Partial<SCMResourceStates>;
if (asResourceStates.resourceStates) {
resources = asResourceStates.resourceStates.map(resource => resource.resourceUri);
} else {
resources = [value.resourceUri];
const asResourceUri = value as SCMResourceUri;
resources = [asResourceUri.resourceUri];
if (additionalSelected) {
resources.push(...additionalSelected.map(resource => resource.resourceUri));
}
Expand Down
30 changes: 19 additions & 11 deletions src/common/logger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ import { Disposable } from './lifecycle';

export const PR_TREE = 'PullRequestTree';

interface Stringish {
toString: () => string;
}

class Log extends Disposable {
private readonly _outputChannel: vscode.LogOutputChannel;
private readonly _activePerfMarkers: Map<string, number> = new Map();
Expand All @@ -28,36 +32,40 @@ class Log extends Disposable {
this._activePerfMarkers.delete(marker);
}

private logString(message: any, component?: string): string {
private logString(message: string | Error | Stringish | Object, component?: string): string {
let logMessage: string;
if (typeof message !== 'string') {
const asString = message as Partial<Stringish>;
if (message instanceof Error) {
message = message.message;
} else if ('toString' in message) {
message = message.toString();
logMessage = message.message;
} else if (asString.toString) {
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

Checking for toString method existence is similar to the 'in' operator usage that this PR aims to eliminate. Consider using a more explicit type checking approach.

Suggested change
} else if (asString.toString) {
} else if (typeof asString.toString === 'function') {

Copilot uses AI. Check for mistakes.
logMessage = asString.toString();
} else {
message = JSON.stringify(message);
logMessage = JSON.stringify(message);
}
} else {
logMessage = message;
}
return component ? `[${component}] ${message}` : message;
return component ? `[${component}] ${logMessage}` : logMessage;
}

public trace(message: any, component: string) {
public trace(message: string | Error | Stringish | Object, component: string) {
this._outputChannel.trace(this.logString(message, component));
}

public debug(message: any, component: string) {
public debug(message: string | Error | Stringish | Object, component: string) {
this._outputChannel.debug(this.logString(message, component));
}

public appendLine(message: any, component: string) {
public appendLine(message: string | Error | Stringish | Object, component: string) {
this._outputChannel.info(this.logString(message, component));
}

public warn(message: any, component?: string) {
public warn(message: string | Error | Stringish | Object, component?: string) {
this._outputChannel.warn(this.logString(message, component));
}

public error(message: any, component: string) {
public error(message: string | Error | Stringish | Object, component: string) {
this._outputChannel.error(this.logString(message, component));
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/common/uri.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import fetch from 'cross-fetch';
import * as vscode from 'vscode';
import { Repository } from '../api/api';
import { EXTENSION_ID } from '../constants';
import { IAccount, isTeam, ITeam, reviewerId } from '../github/interface';
import { IAccount, isITeam, ITeam, reviewerId } from '../github/interface';
import { PullRequestModel } from '../github/pullRequestModel';
import { GitChangeType } from './file';
import Logger from './logger';
Expand Down Expand Up @@ -322,7 +322,7 @@ export namespace DataUri {
await doFetch();
} catch (retryError) {
// Log the error and return a generic user icon instead of crashing
const userIdentifier = isTeam(user) ? `${user.org}/${user.slug}` : user.login || 'unknown';
const userIdentifier = isITeam(user) ? `${user.org}/${user.slug}` : user.login || 'unknown';
Logger.error(`Failed to fetch avatar after retry for user ${userIdentifier}: ${retryError}`, 'avatarCirclesAsImageDataUris');
return genericUserIconAsImageDataURI(width, height);
}
Expand Down
6 changes: 3 additions & 3 deletions src/github/activityBarViewProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { formatError } from '../common/utils';
import { getNonce, IRequestMessage, WebviewViewBase } from '../common/webview';
import { ReviewManager } from '../view/reviewManager';
import { FolderRepositoryManager } from './folderRepositoryManager';
import { GithubItemStateEnum, IAccount, isTeam, ITeam, PullRequestMergeability, reviewerId, ReviewEventEnum, ReviewState } from './interface';
import { GithubItemStateEnum, IAccount, isITeam, ITeam, PullRequestMergeability, reviewerId, ReviewEventEnum, ReviewState } from './interface';
import { PullRequestModel } from './pullRequestModel';
import { getDefaultMergeMethod } from './pullRequestOverview';
import { PullRequestView } from './pullRequestOverviewCommon';
Expand Down Expand Up @@ -154,9 +154,9 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W
}
}

if (targetReviewer && isTeam(targetReviewer.reviewer)) {
if (targetReviewer && isITeam(targetReviewer.reviewer)) {
teamReviewers.push(targetReviewer.reviewer);
} else if (targetReviewer && !isTeam(targetReviewer.reviewer)) {
} else if (targetReviewer && !isITeam(targetReviewer.reviewer)) {
userReviewers.push(targetReviewer.reviewer);
}

Expand Down
4 changes: 2 additions & 2 deletions src/github/createPRViewProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import {
titleAndBodyFrom,
} from './folderRepositoryManager';
import { GitHubRepository } from './githubRepository';
import { IAccount, ILabel, IMilestone, IProject, isTeam, ITeam, MergeMethod, RepoAccessAndMergeMethods } from './interface';
import { IAccount, ILabel, IMilestone, IProject, isITeam, ITeam, MergeMethod, RepoAccessAndMergeMethods } from './interface';
import { BaseBranchMetadata, PullRequestGitHelper } from './pullRequestGitHelper';
import { PullRequestModel } from './pullRequestModel';
import { getDefaultMergeMethod } from './pullRequestOverview';
Expand Down Expand Up @@ -315,7 +315,7 @@ export abstract class BaseCreatePullRequestViewProvider<T extends BasePullReques
const users: IAccount[] = [];
const teams: ITeam[] = [];
for (const reviewer of reviewers) {
if (isTeam(reviewer)) {
if (isITeam(reviewer)) {
teams.push(reviewer);
} else {
users.push(reviewer);
Expand Down
6 changes: 4 additions & 2 deletions src/github/graphql.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,13 @@ export interface Account extends Actor {
}

export function isAccount(x: Actor | Team | Node | undefined | null): x is Account {
return !!x && 'name' in x && 'email' in x;
const asAccount = x as Partial<Account>;
return !!asAccount && !!asAccount?.name && !!asAccount?.email;
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

The logic for checking if an object is an Account is incorrect. It should check if the properties exist (are not undefined), not if they are truthy. An account could have name: '' or email: '', which would make this function return false incorrectly.

Suggested change
return !!asAccount && !!asAccount?.name && !!asAccount?.email;
return !!asAccount && asAccount?.name !== undefined && asAccount?.email !== undefined;

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

The logic requires both name and email to be truthy, but according to the Account interface, email is optional (email?: string). This will incorrectly return false for valid Account objects that don't have an email.

Suggested change
return !!asAccount && !!asAccount?.name && !!asAccount?.email;
return !!asAccount && !!asAccount?.name;

Copilot uses AI. Check for mistakes.
}

export function isTeam(x: Actor | Team | Node | undefined | null): x is Team {
return !!x && 'slug' in x;
const asTeam = x as Partial<Team>;
return !!asTeam && !!asTeam?.slug;
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

The logic for checking if an object is a Team is incorrect. It should check if the slug property exists (is not undefined), not if it is truthy. A team could have slug: '', which would make this function return false incorrectly.

Suggested change
return !!asTeam && !!asTeam?.slug;
return !!asTeam && asTeam?.slug !== undefined;

Copilot uses AI. Check for mistakes.
}

export interface Team {
Expand Down
14 changes: 8 additions & 6 deletions src/github/interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,26 +104,28 @@ export interface MergeQueueEntry {

export function reviewerId(reviewer: ITeam | IAccount): string {
// We can literally get different login values for copilot depending on where it's coming from (already assignee vs suggested assingee)
return isTeam(reviewer) ? reviewer.id : (reviewer.specialDisplayName ?? reviewer.login);
return isITeam(reviewer) ? reviewer.id : (reviewer.specialDisplayName ?? reviewer.login);
}

export function reviewerLabel(reviewer: ITeam | IAccount | IActor | any): string {
return isTeam(reviewer) ? (reviewer.name ?? reviewer.slug ?? reviewer.id) : (reviewer.specialDisplayName ?? reviewer.login);
return isITeam(reviewer) ? (reviewer.name ?? reviewer.slug ?? reviewer.id) : (reviewer.specialDisplayName ?? reviewer.login);
}

export function isTeam(reviewer: ITeam | IAccount | IActor | any): reviewer is ITeam {
return 'org' in reviewer;
export function isITeam(reviewer: ITeam | IAccount | IActor | any): reviewer is ITeam {
const asITeam = reviewer as Partial<ITeam>;
return !!asITeam.org;
}

export interface ISuggestedReviewer extends IAccount {
isAuthor: boolean;
isCommenter: boolean;
}

export function isSuggestedReviewer(
export function isISuggestedReviewer(
reviewer: IAccount | ISuggestedReviewer | ITeam
): reviewer is ISuggestedReviewer {
return 'isAuthor' in reviewer && 'isCommenter' in reviewer;
const asISuggestedReviewer = reviewer as Partial<ISuggestedReviewer>;
return !!asISuggestedReviewer.isAuthor && !!asISuggestedReviewer.isCommenter;
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

The logic for checking if a reviewer is an ISuggestedReviewer is incorrect. It should check if both properties exist (are not undefined), not if they are truthy. A reviewer could have isAuthor: false and isCommenter: false, which would make this function return false incorrectly.

Suggested change
return !!asISuggestedReviewer.isAuthor && !!asISuggestedReviewer.isCommenter;
return typeof asISuggestedReviewer.isAuthor !== 'undefined' && typeof asISuggestedReviewer.isCommenter !== 'undefined';

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

The logic for checking if an object is an ISuggestedReviewer is incorrect. The function should return true when both properties exist (regardless of their boolean values), but the current implementation requires both to be truthy. This could cause false negatives when isAuthor or isCommenter are explicitly false.

Suggested change
return !!asISuggestedReviewer.isAuthor && !!asISuggestedReviewer.isCommenter;
return 'isAuthor' in asISuggestedReviewer && 'isCommenter' in asISuggestedReviewer;

Copilot uses AI. Check for mistakes.
}

export interface IProject {
Expand Down
5 changes: 1 addition & 4 deletions src/github/pullRequestGitHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,10 +340,7 @@ export class PullRequestGitHelper {

static async isRemoteCreatedForPullRequest(repository: Repository, remoteName: string) {
try {
Logger.debug(
`Check if remote '${remoteName}' is created for pull request - start`,
PullRequestGitHelper.ID,
);
Logger.debug(`Check if remote '${remoteName}' is created for pull request - start`, PullRequestGitHelper.ID);
const isForPR = await repository.getConfig(`remote.${remoteName}.${PullRequestRemoteMetadataKey}`);
Logger.debug(`Check if remote '${remoteName}' is created for pull request - end`, PullRequestGitHelper.ID);
return isForPR === 'true';
Expand Down
15 changes: 3 additions & 12 deletions src/github/pullRequestModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1256,20 +1256,14 @@ export class PullRequestModel extends IssueModel<PullRequest> implements IPullRe
commit: OctokitCommon.PullsListCommitsResponseData[0],
): Promise<OctokitCommon.ReposGetCommitResponseFiles> {
try {
Logger.debug(
`Fetch file changes of commit ${commit.sha} in PR #${this.number} - enter`,
PullRequestModel.ID,
);
Logger.debug(`Fetch file changes of commit ${commit.sha} in PR #${this.number} - enter`, PullRequestModel.ID,);
const { octokit, remote } = await this.githubRepository.ensure();
const fullCommit = await octokit.call(octokit.api.repos.getCommit, {
owner: remote.owner,
repo: remote.repositoryName,
ref: commit.sha,
});
Logger.debug(
`Fetch file changes of commit ${commit.sha} in PR #${this.number} - done`,
PullRequestModel.ID,
);
Logger.debug(`Fetch file changes of commit ${commit.sha} in PR #${this.number} - done`, PullRequestModel.ID,);

return fullCommit.data.files ?? [];
} catch (e) {
Expand Down Expand Up @@ -1552,10 +1546,7 @@ export class PullRequestModel extends IssueModel<PullRequest> implements IPullRe
this._onDidChangeChangesSinceReview.fire();
}

Logger.debug(
`Fetch file changes and merge base of PR #${this.number} - done, total files ${files.length} `,
PullRequestModel.ID,
);
Logger.debug(`Fetch file changes and merge base of PR #${this.number} - done, total files ${files.length} `, PullRequestModel.ID,);
return files;
}

Expand Down
14 changes: 7 additions & 7 deletions src/github/pullRequestOverview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { FolderRepositoryManager } from './folderRepositoryManager';
import {
GithubItemStateEnum,
IAccount,
isTeam,
isITeam,
ITeam,
MergeMethod,
MergeMethodsAvailability,
Expand Down Expand Up @@ -420,7 +420,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
quickPick.busy = false;
const acceptPromise: Promise<(IAccount | ITeam)[]> = asPromise<void>(quickPick.onDidAccept).then(() => {
const pickedReviewers: (IAccount | ITeam)[] | undefined = quickPick?.selectedItems.filter(item => item.user).map(item => item.user) as (IAccount | ITeam)[];
const botReviewers = this._existingReviewers.filter(reviewer => !isTeam(reviewer.reviewer) && reviewer.reviewer.accountType === 'Bot').map(reviewer => reviewer.reviewer);
const botReviewers = this._existingReviewers.filter(reviewer => !isITeam(reviewer.reviewer) && reviewer.reviewer.accountType === 'Bot').map(reviewer => reviewer.reviewer);
return pickedReviewers.concat(botReviewers);
});
const hidePromise = asPromise<void>(quickPick.onDidHide);
Expand All @@ -432,15 +432,15 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
const newUserReviewers: IAccount[] = [];
const newTeamReviewers: ITeam[] = [];
allReviewers.forEach(reviewer => {
const newReviewers: (IAccount | ITeam)[] = isTeam(reviewer) ? newTeamReviewers : newUserReviewers;
const newReviewers: (IAccount | ITeam)[] = isITeam(reviewer) ? newTeamReviewers : newUserReviewers;
newReviewers.push(reviewer);
});

const removedUserReviewers: IAccount[] = [];
const removedTeamReviewers: ITeam[] = [];
this._existingReviewers.forEach(existing => {
let newReviewers: (IAccount | ITeam)[] = isTeam(existing.reviewer) ? newTeamReviewers : newUserReviewers;
let removedReviewers: (IAccount | ITeam)[] = isTeam(existing.reviewer) ? removedTeamReviewers : removedUserReviewers;
let newReviewers: (IAccount | ITeam)[] = isITeam(existing.reviewer) ? newTeamReviewers : newUserReviewers;
let removedReviewers: (IAccount | ITeam)[] = isITeam(existing.reviewer) ? removedTeamReviewers : removedUserReviewers;
if (!newReviewers.find(newTeamReviewer => newTeamReviewer.id === existing.reviewer.id)) {
removedReviewers.push(existing.reviewer);
}
Expand Down Expand Up @@ -769,9 +769,9 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
}
}

if (targetReviewer && isTeam(targetReviewer.reviewer)) {
if (targetReviewer && isITeam(targetReviewer.reviewer)) {
teamReviewers.push(targetReviewer.reviewer);
} else if (targetReviewer && !isTeam(targetReviewer.reviewer)) {
} else if (targetReviewer && !isITeam(targetReviewer.reviewer)) {
userReviewers.push(targetReviewer.reviewer);
}

Expand Down
10 changes: 5 additions & 5 deletions src/github/quickPicks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { DataUri } from '../common/uri';
import { formatError } from '../common/utils';
import { FolderRepositoryManager } from './folderRepositoryManager';
import { GitHubRepository, TeamReviewerRefreshKind } from './githubRepository';
import { AccountType, IAccount, ILabel, IMilestone, IProject, isSuggestedReviewer, isTeam, ISuggestedReviewer, ITeam, reviewerId, ReviewState } from './interface';
import { AccountType, IAccount, ILabel, IMilestone, IProject, isISuggestedReviewer, isITeam, ISuggestedReviewer, ITeam, reviewerId, ReviewState } from './interface';
import { IssueModel } from './issueModel';
import { DisplayLabel } from './views';

Expand All @@ -35,7 +35,7 @@ async function getItems<T extends IAccount | ITeam | ISuggestedReviewer>(context
const user = filteredUsers[i];

let detail: string | undefined;
if (isSuggestedReviewer(user)) {
if (isISuggestedReviewer(user)) {
detail = user.isAuthor && user.isCommenter
? vscode.l10n.t('Recently edited and reviewed changes to these files')
: user.isAuthor
Expand All @@ -46,7 +46,7 @@ async function getItems<T extends IAccount | ITeam | ISuggestedReviewer>(context
}

alreadyAssignedItems.push({
label: isTeam(user) ? `${user.org}/${user.slug}` : COPILOT_ACCOUNTS[user.login] ? COPILOT_ACCOUNTS[user.login].name : user.login,
label: isITeam(user) ? `${user.org}/${user.slug}` : COPILOT_ACCOUNTS[user.login] ? COPILOT_ACCOUNTS[user.login].name : user.login,
description: user.name,
user,
picked,
Expand Down Expand Up @@ -123,13 +123,13 @@ export async function getAssigneesQuickPickItems(folderRepositoryManager: Folder
}

function userThemeIcon(user: IAccount | ITeam) {
return (isTeam(user) ? new vscode.ThemeIcon('organization') : new vscode.ThemeIcon('account'));
return (isITeam(user) ? new vscode.ThemeIcon('organization') : new vscode.ThemeIcon('account'));
}

async function getReviewersQuickPickItems(folderRepositoryManager: FolderRepositoryManager, remoteName: string, isInOrganization: boolean, author: IAccount, existingReviewers: ReviewState[],
suggestedReviewers: ISuggestedReviewer[] | undefined, refreshKind: TeamReviewerRefreshKind,
): Promise<(vscode.QuickPickItem & { user?: IAccount | ITeam })[]> {
existingReviewers = existingReviewers.filter(reviewer => isTeam(reviewer.reviewer) || (reviewer.reviewer.accountType !== AccountType.Bot));
existingReviewers = existingReviewers.filter(reviewer => isITeam(reviewer.reviewer) || (reviewer.reviewer.accountType !== AccountType.Bot));
if (!suggestedReviewers) {
return [];
}
Expand Down
Loading