Skip to content

Conversation

joefarebrother
Copy link
Contributor

@joefarebrother joefarebrother commented Aug 25, 2025

Adds to the shared library parameterised modules for Option types with location support.

Potential for bikeshedding on the names, especially LocOption2; unsure of a better name to describe its purpose.

LocOption requires a type that has a hasLocationInfo predicate, and LocOption2 requires a type with a getLocation predicate, and also needs to know about its result Location type.

This may be useful for queries with variable-length alert messages.
This is applicable to 2 in-flight PRs for Python quality queries (and is cherry-picked from one of them); these PRs will be rebased to depend on this one shortly: #20217, #19932.

I'm not that familiar with the use of parameterised modules, so if there are clearer ways to handle the implementation that I've missed, let me know.

module LocOption<TypeWithLocationInfo T> {
private module O = Option<T>;

final private class BOption = O::Option;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's with the B prefix? Is it meant to be shorthand for Base - in that case just use Base as a prefix - that's slightly clearer, I think. Alternatively, don't introduce final aliases at all - just use instanceof instead of extends, then we'll just need to repeat the 3 member methods with delegating super calls instead. I guess it's mostly a minor matter of style whether to go with extends final or instanceof.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, Base was the intention.
Using final aliases like this felt cleaner than repeating each method.

* For more information, see
* [Providing locations in CodeQL queries](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
*/
predicate hasLocationInfo(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use getLocation here instead of hasLocationInfo and rely on the existence of the empty Location - we do that plenty of other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the canonical preferred way to refer to the empty location?
Is it l.hasLocationInfo("", 0, 0, 0, 0)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. See e.g. git grep 'hasLocationInfo("", 0, 0, 0, 0)'.

@joefarebrother
Copy link
Contributor Author

Suggestions applied.

@aschackmull
Copy link
Contributor

Suggestions applied.

I'm not seeing any changes - did you forget to either commit or push?

@Copilot Copilot AI review requested due to automatic review settings August 26, 2025 14:00
@joefarebrother
Copy link
Contributor Author

Whoops 😅
The latter.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds parameterized modules for Option types with location support to the shared library. The purpose is to provide Option types that can carry location information, which is useful for queries with variable-length alert messages.

  • Adds OptionWithLocationInfo module for types with hasLocationInfo predicate
  • Adds LocatableOption module for types with getLocation predicate
  • Includes appropriate change notes documentation

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
shared/util/codeql/util/Option.qll Adds two new parameterized modules with location support and required type signatures
shared/util/change-notes/2025-08-25-loc-option.md Documents the addition of the new Option modules

}

/** The singleton `None` element. */
class None extends Option instanceof O::Some { }
Copy link
Preview

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

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

The None class incorrectly extends O::Some instead of O::None. This should be class None extends Option instanceof O::None { } to properly represent the None case.

Suggested change
class None extends Option instanceof O::Some { }
class None extends Option instanceof O::None { }

Copilot uses AI. Check for mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copilot is right; the two cases have been swapped.

Comment on lines 98 to 101
class None extends Option instanceof O::Some { }

/** A wrapper for the given type. */
class Some extends Option instanceof O::None { }
Copy link
Preview

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

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

The Some class incorrectly extends O::None instead of O::Some. This should be class Some extends Option instanceof O::Some { } to properly represent the Some case.

Suggested change
class None extends Option instanceof O::Some { }
/** A wrapper for the given type. */
class Some extends Option instanceof O::None { }
class None extends Option instanceof O::None { }
/** A wrapper for the given type. */
class Some extends Option instanceof O::Some { }

Copilot uses AI. Check for mistakes.

}

/** The singleton `None` element. */
class None extends Option instanceof O::Some { }
Copy link
Preview

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

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

The None class incorrectly extends O::Some instead of O::None. This should be class None extends Option instanceof O::None { } to properly represent the None case.

Suggested change
class None extends Option instanceof O::Some { }
class None extends Option instanceof O::None { }

Copilot uses AI. Check for mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

Comment on lines 140 to 143
class None extends Option instanceof O::Some { }

/** A wrapper for the given type. */
class Some extends Option instanceof O::None { }
Copy link
Preview

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

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

The Some class incorrectly extends O::None instead of O::Some. This should be class Some extends Option instanceof O::Some { } to properly represent the Some case.

Suggested change
class None extends Option instanceof O::Some { }
/** A wrapper for the given type. */
class Some extends Option instanceof O::None { }
class None extends Option instanceof O::None { }
/** A wrapper for the given type. */
class Some extends Option instanceof O::Some { }

Copilot uses AI. Check for mistakes.

@@ -45,3 +57,91 @@ module Option<TypeWithToString T> {
/** Gets the given element wrapped as an `Option`. */
Copy link
Preview

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

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

Copilot detected a code snippet with 13 occurrences. See search results for more details.

Matched Code Snippet
*
     * Holds if this element is at the specified location.
     * The location spans column `startColumn` of line `startLine` to
     * column `endColumn` of line `endLine` in file `filepath`.
     * For more information, see
     * [Providing locations in CodeQL queries](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
     */
    predicate hasLocationInfo(
      string filePath, int startLine, int

Copilot uses AI. Check for mistakes.

}

/** The singleton `None` element. */
class None extends Option instanceof O::Some { }
Copy link
Contributor

Choose a reason for hiding this comment

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

Copilot is right; the two cases have been swapped.

}

/** The singleton `None` element. */
class None extends Option instanceof O::Some { }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

LGTM now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants