MutableValue.Exists not accessible

[huan086]

There is a naming conflict between the field MutableValue.Exists and method MutableValue.Exists(). MutableValue.Exists thus cannot be assigned to.

Hoping that FlexLucene detects and renames the fields during build.

[seemantr]

@huan086 Thanks for raising this issue and apologizes for the delay in responding to it. Just wet live with a massive project at work and it seems to be taking up all my time. I will have a look at it when we will produce the binary for Lucene 5.4. Not sure if we can do much apart from not renaming the property in such cases. We can definitely check if the rename results in a property with the same name as an existing method. I will investigate it further now.

[huan086]

Understand. I haven't had the time to look at the source code as well.

Was migrating my code from GDI to DirectWrite using https://github.com/sharpdx/SharpDX. You might want to take a look at their build tools, which takes XML configuration, read C header files and produces the C# code to interface. Might be overkill for Lucene though.

[seemantr]

@huan086 I have fixed this by renaming the field in case of naming conflict. Originally I wanted to fix the method but that is not always viable as the name might be coming from a base class. But renaming the field always works. I have a test named 'MutableValueDoesNotHaveDuplicateMethods' to test your specific case. I have just pushed a new dll in the artifacts folder of the repo. Can you please see if this resolves your issues? After some more testing I will release it on nuget.

[huan086]

Looks like it's not just this that conflicts. Found another conflict in 5.4 with IndexInput.Clone(), which has different return types. Wonder how these conflicts can be detected automatically...

    public virtual IndexInput Clone();
    [EditorBrowsable(EditorBrowsableState = EditorBrowsableState.Never)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    [Modifiers(Modifiers.Public | Modifiers.Synthetic)]
    public override object clone();
    [EditorBrowsable(EditorBrowsableState.Never)]
    [Modifiers(Modifiers.Public | Modifiers.Synthetic)]
    public override object Clone();

[seemantr]

@huan086 Thanks for reporting this. At least we are making some progress :-). After checking the code I can confirm your finding. At best we can get rid of the third Clone method as it is generated post IKVM using Cecil.

Let me try to explain what is happening here:

If a method is an implementation of a method from java.lang then we don't rename it as it will break IKVM. So, in these cases we add a new helper method which has the same signature but is PascalCased. This is what is happening in this case. We encounter clone() which we can't rename so we add another method with same signature and hide the original one using EditorBrowsableState.Never.

Here, I believe I should make an additional check that if the original method is already hidden from the intellisense then there is no need to generate a helper method for it. Actually you can see that the method ended up getting two EditorBrowsable attributes because of this bug.

So, I will fix this. But there will still be two clone methods as these are needed by IKVM and there is not much I can do about them. IKVM already hides the one which returns an object.

Would you mind telling me how this is affecting you? Are you using an editor which is not honouring EditorBrowsable attribute?

On a side note are you aware of any good open source static code analysis tool which I can plug into the build chain to further check the generated code?

[huan086]

I'm trying to inherit from IndexInput to read from another data source (in my case Azure Storage). Overriding the Clone method gives the warning "Cannot change the return type when overriding object FlexLucene.Store.IndexInput.Clone()."

As for the MutableValue, I'm inheriting from DoubleDocValues to add some computation before passing to the scorer. One of the methods I need to override is FunctionValuesValueFiller GetValueFiller, which I initialize a MutableValueDouble and change the fields.

[huan086]

Can't find any static analysis tool... I guess unit testing is the only way :(

[huan086]

Most of my use case is to inherit (Lucene is meant to be easily customizable) and change some functionality. Having IndexInput.Close() call close() is very good, cuz I'll only need to override close(). However, MutableValue.Equals duplicate code from MutableValue.equals, which makes inheriting very troublesome.

For the Clone case, it's kind of bad because Java allows changing return type, while C# doesn't. http://stackoverflow.com/questions/14694852/can-overridden-methods-differ-in-return-type http://stackoverflow.com/questions/7282737/overriding-method-to-change-return-type

Looks like for Clone, there needs to be some Cecil magic https://www.simple-talk.com/blogs/2010/07/19/implementing-method-override-covariance-on-c-part-3/

[seemantr]

@huan086 Thanks for the explanation. It helped me in understanding the issue further. For equals (corresponds to Equals in .net) and hashcode (corresponds to GetHashCode in .net) we can do the same thing as we have done for close that is the .net method (PascalCase) can call the java (camelCase) method.

The reason I didn't do it was I felt it was needed and simple copy will do. I think you don't have to implement all the four methods to implement equality that is equals, Equals, hashCode and GetHashCode. Only implementing the java ones will do the trick that is hashCode and equals. When you call the .net methods they will automatically call the java's methods. This works out of box in IKVM as these methods are mapped out of the box. The only reason I copy them over is to get better performance. I did some micro benchmark and having these implemented improved performance slightly.

I have added the below test to the Smoke tests to illustrate my point. See if this makes sense or I have overlooked something obvious?

type DummyIndexInput(dummy : string) = inherit IndexInput(dummy) override .Length() = 0L override .Seek(l) = () override .close() = () override .GetFilePointer() = 0L override this.Slice(a, b, c) = this :> IndexInput override .ReadBytes(a, b, c) = () override .ReadByte() = 1uy override this.equals(a) = a.GetHashCode() = this.GetHashCode() override __.hashCode() = dummy.Length

let GetHashCodeCallshashCodeBehindTheScene() = let a = new DummyIndexInput("test1") let b = new DummyIndexInput("test02") let c = new DummyIndexInput("test1") printfn "Hash Code tests" printfn "a GetHashCode: %i" <| a.GetHashCode() printfn "a hashCode: %i" <| a.hashCode() printfn "b GetHashCode: %i" <| b.GetHashCode() printfn "b hashCode: %i" <| b.hashCode() printfn "c GetHashCode: %i" <| c.GetHashCode() printfn "c hashCode: %i" <| c.hashCode() Assert.True(a.GetHashCode() = a.hashCode()) Assert.True(b.GetHashCode() = b.hashCode()) Assert.True(c.GetHashCode() = c.hashCode()) Assert.False(a.GetHashCode() = b.GetHashCode()) Assert.False(a.hashCode() = b.hashCode()) Assert.True(a.GetHashCode() = c.GetHashCode()) Assert.True(a.hashCode() = c.hashCode()) Assert.True(a.Equals(c)) Assert.True(a.equals(c)) Assert.True(c.Equals(a)) Assert.True(c.equals(a)) Assert.False(a.Equals(b)) Assert.False(a.equals(b))

Regarding the Clone issue, I am still trying to get my head around it. I can understand the covariant return type concept, but I still don't get how IKVM is doing it. I can see a bridge method in the generated code. Maybe that is where the magic happens. Have to dig it a bit more to see what is happening?

Do you think you can write a small test case to illustrate the issue please? This will help me in testing your exact scenario.

[huan086]

The Equals case is for MutableValue. Overriding equals itself is not enough, since Equals is copied. If Equals calls equals, then Equals does not need to be virtual and overriding equals will do.

I think usuability is more important than optimization in this case...

[huan086]

Going to sleep soon, let me get some parts out first

public class AzureDirectory : Directory
    ...
    public AzureDirectory()
    {
        this.cacheDirectory = FSDirectory.Open(Paths.get(catalogPath));
    }
    ...
    public override IndexInput OpenInput(string name, IOContext context)
    {
        AzureIndexInput input = new AzureIndexInput(name, () => this.cacheDirectory.OpenInput(name, context), () => this.cacheDirectory.CreateOutput(name, context));
            return input;
    }

public class AzureIndexInput : IndexInput
{
    ...
    public AzureIndexInput(string resourceDescription, Func<IndexInput> cacheInputFactory, Func<IndexOutput> outputOfCacheInputFactory, CloudBlockBlob blob)
        : base(resourceDescription)
    {
        this.resourceDescription = resourceDescription;
        this.cacheInputFactory = cacheInputFactory;
        this.outputOfCacheInputFactory = outputOfCacheInputFactory;
    }

    // This causes a warning
    public override IndexInput Clone()
    {
        AzureIndexInput clone = new AzureIndexInput(this.resourceDescription, this.cacheInputFactory, this.outputOfCacheInputFactory);
        clone.Seek(this.GetFilePointer());
        lock (this.clones)
        {
            this.clones.Add(clone);
        }

        return clone;
    }
}

The methods in AzureIndexInput mostly just calls the IndexInput that is passed to it.

[vladnega]

@huan086 I've added a new smoke test in which I create a class that derives from IndexInput called DerivedIndexInput. When overriding Clone(), I return a new instance of the derived class, just as you are doing in your AzureIndexInput example. The only difference is that I explicitly cast my return object to IndexInput.

I looked in the Error List and I can see no warnings showing for this override. Maybe there is something I'm not quite getting, or maybe the issue has been solved by previous fixes? Or maybe because I can't see this method any more on IndexInput?

[EditorBrowsable(EditorBrowsableState.Never)]
[Modifiers(Modifiers.Public | Modifiers.Synthetic)]
public override object Clone();

Could you please check?

[huan086]

The warning is gone. Everything looks perfect! Thanks!