Add operator overrides for public IComparable types, #683 #1056

paulirwin · 2024-12-04T05:03:52Z

You've read the Contributor Guide and Code of Conduct.
You've included unit or integration tests for your change, where applicable.
You've included inline docs for your change, where applicable.
There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Add operator overrides for public IComparable types

Fixes #683

Description

This is to resolve the Sonar warning about needing to override the comparison operators for any types that implement IComparable. We are only doing this here for the public types.

QualityQuery was missing Equals and GetHashCode, as well as had some possible NullReferenceExceptions, so those have been fixed as well.

paulirwin · 2024-12-04T15:55:30Z

This change caught a NRE bug in MutableValue 😄 Fixed.

NightOwl888 · 2024-12-04T17:02:06Z

This is a draft PR, because I'm currently unsure about whether to override Equals and GetHashCode (and if so, how exactly to do that, i.e. what to compare) for QualityQuery. @NightOwl888 - please let me know your thoughts.

Well, being that they didn't override the default behavior (which is reference equality), a direct port would be to override each and then cascade the call to the base class.

But given the fact that the comparer uses queryID and it says "ID of this quality query" for a description, it would probably be best to use that to compare for equality/hash code. It would keep the equality and compare checks exactly in sync.

BTW - I noticed that the QualityQuery.CompareTo() implementation has an exception handler that we can remove because we can use int.TryParse() and fallback to string comparison if parsing either side returns false.

        public virtual int CompareTo(QualityQuery other)
        {
            try
            {
                // compare as ints when ids ints
                int n = int.Parse(queryID, CultureInfo.InvariantCulture);
                int nOther = int.Parse(other.queryID, CultureInfo.InvariantCulture);
                return n - nOther;
            }
            catch (Exception e) when (e.IsNumberFormatException())
            {
                // fall back to string comparison
                return queryID.CompareToOrdinal(other.queryID);
            }
        }

The constructor could also be improved by using guard clauses on queryID and nameValPairs since there will be NREs if either is passed null. If we do so, we don't need to change CompareTo() to account for the fact that queryID may be null (which it currently doesn't handle).

#nullable enable would catch this sort of thing.

NightOwl888

Thanks for the PR.

We are getting some build warnings in some types where Equals(object) and GetHashCode() have not been implemented.

Also, since these are all on nullable types, please use #nullable enable within the #region Operator overrides sections and #nullable restore prior to #endregion. We don't necessarily have to do this on the entire file in this PR.

See the inline comments.

src/Lucene.Net.Benchmark/Quality/QualityQuery.cs

src/Lucene.Net.QueryParser/Surround/Query/SimpleTerm.cs

src/Lucene.Net.Suggest/Suggest/Fst/FSTCompletion.cs

src/Lucene.Net.Suggest/Suggest/Lookup.cs

… CompareTo implementation

NightOwl888 · 2024-12-13T02:58:17Z

src/Lucene.Net.Highlighter/VectorHighlight/FieldPhraseList.cs

@@ -470,6 +470,31 @@ public override bool Equals(object obj)
                return true;
            }

+            #region Operator overrides
+            #nullable enable


By default, Visual Studio left aligns #nullable enable and #nullable restore, so please do that in each case in this PR.

NightOwl888 · 2024-12-13T02:59:29Z

src/Lucene.Net.QueryParser/Surround/Query/SimpleTerm.cs

@@ -2,6 +2,7 @@
 using Lucene.Net.Index;
 using System;
 using System.Text;
+#pragma warning disable CS0660, CS0661 - CompareTo is deprecated, so skipping implementing equality members (lucenenet#683)


Please fix the inline comment by preceeding with //.

NightOwl888 · 2024-12-13T03:03:45Z

src/Lucene.Net.Suggest/Suggest/Fst/FSTCompletion.cs

+
+            public override int GetHashCode()
+            {
+                unchecked


Please cascade the call to the base class.

public override int GetHashCode() => base.GetHashCode();

NightOwl888 · 2024-12-13T03:04:39Z

src/Lucene.Net.Suggest/Suggest/Fst/FSTCompletion.cs

@@ -67,6 +67,42 @@ public int CompareTo(Completion o)
            {
                return this.Utf8.CompareTo(o.Utf8);
            }
+
+            // LUCENENET specific - per CS0660 and CS0661, we need to override Equals and GetHashCode
+            public override bool Equals(object obj) => ReferenceEquals(this, obj);


Please cascade the call to the base class.

public override bool Equals(object obj) => base.Equals(obj);

NightOwl888 · 2024-12-13T03:04:52Z

src/Lucene.Net.Suggest/Suggest/Lookup.cs

@@ -122,14 +122,53 @@ public int CompareTo(LookupResult o)
            {
                return CHARSEQUENCE_COMPARER.Compare(Key, o.Key);
            }
+
+            // LUCENENET specific - per CS0660 and CS0661, we need to override Equals and GetHashCode
+            public override bool Equals(object obj) => ReferenceEquals(this, obj);


Please cascade the call to the base class.

public override bool Equals(object obj) => base.Equals(obj);

NightOwl888 · 2024-12-13T03:05:16Z

src/Lucene.Net.Suggest/Suggest/Lookup.cs

+            // LUCENENET specific - per CS0660 and CS0661, we need to override Equals and GetHashCode
+            public override bool Equals(object obj) => ReferenceEquals(this, obj);
+
+            public override int GetHashCode()


Please cascade the call to the base class.

public override int GetHashCode() => base.GetHashCode();

NightOwl888 · 2024-12-13T03:06:51Z

src/Lucene.Net/Util/Automaton/State.cs

        // Although it doesn't match GetHashCode(), checking for
        // reference equality is by design.
        // Implementing Equals() causes difficult to diagnose
        // IndexOutOfRangeExceptions when using FuzzyTermsEnum.
        // See GH-296.
+        // Overriding here to prevent CS0660 warning due to defining the == operator.
+        public override bool Equals(object obj) => ReferenceEquals(this, obj);


Please cascade the call to the base class.

public override bool Equals(object obj) => base.Equals(obj);

paulirwin added the notes:improvement An enhancement to an existing feature label Dec 4, 2024

paulirwin requested a review from NightOwl888 December 4, 2024 05:03

paulirwin marked this pull request as ready for review December 4, 2024 22:16

NightOwl888 requested changes Dec 12, 2024

View reviewed changes

paulirwin added 4 commits December 12, 2024 15:36

Add operator overrides for IComparable types, apache#683

6a45ecf

Fix null reference exception in MutableValue.Equals

5cc60ad

Add Equals and GetHashCode to QualityQuery, enavle nullable, clean up…

bdb3d0e

… CompareTo implementation

PR feedback

14fa75d

paulirwin force-pushed the issue/683 branch from 939d7bb to 14fa75d Compare December 12, 2024 22:37

paulirwin requested a review from NightOwl888 December 12, 2024 22:44

NightOwl888 requested changes Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add operator overrides for public IComparable types, #683 #1056

Add operator overrides for public IComparable types, #683 #1056

paulirwin commented Dec 4, 2024 •

edited

Loading

paulirwin commented Dec 4, 2024

NightOwl888 commented Dec 4, 2024

NightOwl888 left a comment

NightOwl888 Dec 13, 2024

NightOwl888 Dec 13, 2024

NightOwl888 Dec 13, 2024

NightOwl888 Dec 13, 2024

NightOwl888 Dec 13, 2024

NightOwl888 Dec 13, 2024

NightOwl888 Dec 13, 2024

Add operator overrides for public IComparable types, #683 #1056

Are you sure you want to change the base?

Add operator overrides for public IComparable types, #683 #1056

Conversation

paulirwin commented Dec 4, 2024 • edited Loading

Description

paulirwin commented Dec 4, 2024

NightOwl888 commented Dec 4, 2024

NightOwl888 left a comment

Choose a reason for hiding this comment

NightOwl888 Dec 13, 2024

Choose a reason for hiding this comment

NightOwl888 Dec 13, 2024

Choose a reason for hiding this comment

NightOwl888 Dec 13, 2024

Choose a reason for hiding this comment

NightOwl888 Dec 13, 2024

Choose a reason for hiding this comment

NightOwl888 Dec 13, 2024

Choose a reason for hiding this comment

NightOwl888 Dec 13, 2024

Choose a reason for hiding this comment

NightOwl888 Dec 13, 2024

Choose a reason for hiding this comment

paulirwin commented Dec 4, 2024 •

edited

Loading