Skip to content

C#: Fix some LGTM alerts on the extractor - #755

Merged
hvitved merged 4 commits into
github:masterfrom
calumgrant:cs/extractor-alerts
Jan 14, 2019
Merged

C#: Fix some LGTM alerts on the extractor#755
hvitved merged 4 commits into
github:masterfrom
calumgrant:cs/extractor-alerts

Conversation

@calumgrant

Copy link
Copy Markdown
Contributor

No description provided.

@calumgrant
calumgrant requested a review from a team as a code ownerJanuary 11, 2019 12:48
@pavgust

Copy link
Copy Markdown
Contributor

This pull request fixes 5 alerts when merging de4f592 into b0dd3df - view on LGTM.com

fixed alerts:

  • 3 for Dereferenced variable may be null
  • 1 for Redundant ToString() call
  • 1 for Reference equality test on System.Object

Comment posted by LGTM.com

@hvitvedhvitved left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, just two comments.

{
var other = obj as CachedEntity<Initializer>;
return obj != null && obj.GetType() == GetType() && Equals(other.symbol, symbol);
return other != null && other.GetType() == GetType() && Equals(other.symbol, symbol);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could replace other != null && other.GetType() == GetType() with other?.GetType() == GetType()

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good idea.

}
}
catch (Exception ex)
catch (Exception ex) // lgtm[cs/catch-of-all-exceptions]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps we should disable this query in the .lgtm.yml file instead?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think leave this, because it would be in principle a good idea to receive new alerts for cs/catch-of-all-exceptions if we introduce new ones.

@pavgust

Copy link
Copy Markdown
Contributor

This pull request fixes 4 alerts when merging e76eb16 into dcdbc00 - view on LGTM.com

fixed alerts:

  • 2 for Dereferenced variable may be null
  • 1 for Redundant ToString() call
  • 1 for Reference equality test on System.Object

Comment posted by LGTM.com

@hvitved
hvitved merged commit bbc49dc into github:masterJan 14, 2019
nodeselector added a commit to nodeselector/codeql that referenced this pull request Aug 31, 2026
Adds the seam for making actions/unpinned-tag aware of a repository's Actions
lockfile (.github/workflows/actions.lock), so that a tag ref bound to a verified
commit in the lockfile is not reported as unpinned (Option A from the github#755
spike).
Introduces the extensible predicate
pinnedByLockfileDataModel(workflow_path, nwo, ref) in ConfigExtensions.qll,
re-exported through Config.qll, with a data-extension stub in
ext/config/pinned_by_lockfile.yml documenting the intended row shape. The query
gains a "not pinnedByLockfile(...)" clause keyed on the workflow file's relative
path.
The predicate is meant to be populated by the CodeQL Actions extractor, which
must parse actions.lock at database-creation time using the canonical parser
github.com/github/actions-lockfile/go. That extractor work is a separate change
and is not implemented here; until it ships the predicate is empty and the new
clause is a no-op. A test-scoped data extension exercises the clause end to end.
Part of github/actions-dispatch#755.
nodeselector added a commit to nodeselector/codeql that referenced this pull request Sep 2, 2026
Adds the seam for making actions/unpinned-tag aware of a repository's Actions
lockfile (.github/workflows/actions.lock), so that a tag ref bound to a verified
commit in the lockfile is not reported as unpinned (Option A from the github#755
spike).
Introduces the extensible predicate
pinnedByLockfileDataModel(workflow_path, nwo, ref) in ConfigExtensions.qll,
re-exported through Config.qll, with a data-extension stub in
ext/config/pinned_by_lockfile.yml documenting the intended row shape. The query
gains a "not pinnedByLockfile(...)" clause keyed on the workflow file's relative
path.
The predicate is meant to be populated by the CodeQL Actions extractor, which
must parse actions.lock at database-creation time using the canonical parser
github.com/github/actions-lockfile/go. That extractor work is a separate change
and is not implemented here; until it ships the predicate is empty and the new
clause is a no-op. A test-scoped data extension exercises the clause end to end.
Part of github/actions-dispatch#755.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@calumgrant@pavgust@hvitved