NightOwl888 commented on issue #648: URL: https://github.com/apache/lucenenet/issues/648#issuecomment-1299613751
> @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. > > ```cs > 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. I guess I am also going to have to state that this bug is probably going to be helpful to find all of the protected APIs that were translated from Java to C# wrong. So, we should wait until #677 is closed before we report it to SonarCloud. -- 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-unsubscr...@lucenenet.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
