[GitHub] [lucenenet] NightOwl888 commented on issue #648: Which kinds of PR's would you like to get?

[GitBox <gi...@apache.org>]

NightOwl888 commented on issue #648:
URL: https://github.com/apache/lucenenet/issues/648#issuecomment-1299606844

   @nikcio - I found what appears to be a bug in the scan: https://sonarcloud.io/project/issues?issues=AYPAuOCLhbfJOGLOoaG2&open=AYPAuOCLhbfJOGLOoaG2&id=nikcio_lucenenet
   
   The scan is recommending to convert from [`protected internal`](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/protected-internal) to [`private protected`](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/private-protected). The former allows internal (public-like) access within the assembly and protected access outside of the assembly. The latter allows protected access within the assembly and **no access** outside of the assembly.
   
   I double-checked, and indeed it is possible to inherit `FieldCacheRangeFilter<T>` outside of the assembly.
   
   ```c#
       public class Foo : FieldCacheRangeFilter<double>
       {
           public Foo()
               : base("foo", FieldCache.NUMERIC_UTILS_DOUBLE_PARSER, lowerVal: 444.444, upperVal: 555.555, includeLower: true, includeUpper: true)
           {
           }
   
           public override DocIdSet GetDocIdSet(AtomicReaderContext context, IBits acceptDocs)
           {
               throw new NotImplementedException();
           }
       }
   ```
   
   Clearly the advice given by the scan is incorrect if we want this to remain a `protected` constructor outside of the assembly.
   
   That being said, checking with the [Lucene 4.8.0 source](https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.0/lucene/core/src/java/org/apache/lucene/search/FieldCacheRangeFilter.java#L59-L71) the constructor is actually declared `private`, so this is also a Java to C# translation bug and it should ultimately be made `private protected` as the scan indicated.
   
   > **NOTE:** `protected internal` was initially put on all `protected` APIs because in Java it is possible to call a protected member within the same package like it is public. However, `protected internal` is a big mess when it is combined with toggling on and off `InternalsVisibleToAttribute` as required. Once `InternalsVisibleToAttribute` is enabled, all of the subclasses need to be changed from `protected` to `protected internal` to match accessibility. Ideally, we would avoid `protected internal` and use `protected` when we can, but since some internal places use these APIs like they are public, we need it in those specific cases.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org