Details

    • Type: Task Task
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: Lucene.Net 2.9.4
    • Fix Version/s: Lucene.Net 3.6
    • Component/s: Lucene.Net Core
    • Labels:
      None

      Description

      Make Lucene.Net CLS Compliant

        Issue Links

          Activity

          Hide
          Prescott Nasser added a comment -

          This is a partial fix (it will allow Lucene to build, but there are infinite loops) - however, it has horrible conseqenses and should not be used.

          Here's the jist of what's been done in this patch:

          • uint (UInt32) -> Int64 (long)
          • sbyte -> Int16 (short)
            There are a few sbyte values left, but it's relating to the FieldCacheImpl and used as a key in the dictionary (as a string value) so this should be fine
          • Const fields cannot have _ as first character, removed _ and appended internal or protected to the name if neccessary
          • Volatile, is CLS complaint, but mostly only non CLS Compliant fields can be marked as volatile (exception int). Most fields were uint and sbyte, so those were
            changed to the .net cls compliant version, and locks were put in where more than a single read or write to these variables was occuring
          • Differing only in case is not CLS Compliant. I appended internal (or protected if the field was not marked as internal) to field definitions

          Issues:

          • ulong (UInt64) -> decimal, Lucene.Net.QueryParsers.QueryParserTokenManager didn't like a lot of the Decimal conversions, so I didn't make the ulong -> decimal type change.
            Oddly, there are no CLS compliance warnings from the ulongs here.
          • Lucene.Net.QueryParsers.QueryParser.Term - differing only in case - but couldn't find the other term with different case
          • Lucene.Net.Search.TopDocs.scoreDocs and ScoreDocs - I could not adjust these, without changing the public interface for Luncene.Net
          • Lucene.Net.Search.TopDocs.totalHits and TotalHits - same issue as ScoreDoc
          • Lucene.Net.Search.Similarity idfExplain() and IdfExplain - same issue as ScoreDoc
          Show
          Prescott Nasser added a comment - This is a partial fix (it will allow Lucene to build, but there are infinite loops) - however, it has horrible conseqenses and should not be used. Here's the jist of what's been done in this patch: uint (UInt32) -> Int64 (long) sbyte -> Int16 (short) There are a few sbyte values left, but it's relating to the FieldCacheImpl and used as a key in the dictionary (as a string value) so this should be fine Const fields cannot have _ as first character, removed _ and appended internal or protected to the name if neccessary Volatile, is CLS complaint, but mostly only non CLS Compliant fields can be marked as volatile (exception int). Most fields were uint and sbyte, so those were changed to the .net cls compliant version, and locks were put in where more than a single read or write to these variables was occuring Differing only in case is not CLS Compliant. I appended internal (or protected if the field was not marked as internal) to field definitions Issues: ulong (UInt64) -> decimal, Lucene.Net.QueryParsers.QueryParserTokenManager didn't like a lot of the Decimal conversions, so I didn't make the ulong -> decimal type change. Oddly, there are no CLS compliance warnings from the ulongs here. Lucene.Net.QueryParsers.QueryParser.Term - differing only in case - but couldn't find the other term with different case Lucene.Net.Search.TopDocs.scoreDocs and ScoreDocs - I could not adjust these, without changing the public interface for Luncene.Net Lucene.Net.Search.TopDocs.totalHits and TotalHits - same issue as ScoreDoc Lucene.Net.Search.Similarity idfExplain() and IdfExplain - same issue as ScoreDoc
          Hide
          Prescott Nasser added a comment -

          Additionally, all shifting is broken (this causes an infinite loop in parts of the code - specifically the parts that write to file store (affectively making this patch useless in it's current form).

          The problem is that uint is not CLS compliant, and updating it to CLS Complaint (Int64) breaks shifting. Normally a -1 as a uint, would be 4Billion etc, however as an Int64, it is still -1. This means that we would have to adjust nearly all of the code relating to shifting, which is above my knowledge at the moment. Even making these changes would in my head reset our current status of having a relatively battle tested 2.9.4 release ready to go.

          Further, the fact that we can't fix a few of the mentioned items above (scoreDocs vs ScoreDocs, etc) we cannot make 2.9.4 CLS compliant without breaking the public api.

          With these issues, I think we should abandon the idea of making 2.9.4 CLS Compliant.

          Show
          Prescott Nasser added a comment - Additionally, all shifting is broken (this causes an infinite loop in parts of the code - specifically the parts that write to file store (affectively making this patch useless in it's current form). The problem is that uint is not CLS compliant, and updating it to CLS Complaint (Int64) breaks shifting. Normally a -1 as a uint, would be 4Billion etc, however as an Int64, it is still -1. This means that we would have to adjust nearly all of the code relating to shifting, which is above my knowledge at the moment. Even making these changes would in my head reset our current status of having a relatively battle tested 2.9.4 release ready to go. Further, the fact that we can't fix a few of the mentioned items above (scoreDocs vs ScoreDocs, etc) we cannot make 2.9.4 CLS compliant without breaking the public api. With these issues, I think we should abandon the idea of making 2.9.4 CLS Compliant.
          Hide
          Christopher Currens added a comment -

          I've decided to start work on this issue again. I'm not going to go about and change all of the signed vs unsigned types for so many reasons. I'm going to focus on just making naming CLS compliant, really just so it can be used in case-insensitive languages, like VB.

          The problem is that may of the variables are public (they are not public in java, and shouldn't be in .NET), so I really have no way of knowing if they are being used in external code. We have Obsolete attributes on SOME of them, but not all. Should we push through this and just make the changes or wait for another release and mark them obsoleted in the meantime?

          Show
          Christopher Currens added a comment - I've decided to start work on this issue again. I'm not going to go about and change all of the signed vs unsigned types for so many reasons. I'm going to focus on just making naming CLS compliant, really just so it can be used in case-insensitive languages, like VB. The problem is that may of the variables are public (they are not public in java, and shouldn't be in .NET), so I really have no way of knowing if they are being used in external code. We have Obsolete attributes on SOME of them, but not all. Should we push through this and just make the changes or wait for another release and mark them obsoleted in the meantime?
          Hide
          Christopher Currens added a comment -

          I was looking at this again, and we can be CLSCompliant and leave all of that FieldsWriter shifting alone.

          Being CLS compliant only applies to public and protected members. We can do whatever we want with internal or private members, including using non-CLS compliant types. On top of there being a lot of methods marked public that should be internal or protected internal, I think this might be easier than we originally thought.

          Show
          Christopher Currens added a comment - I was looking at this again, and we can be CLSCompliant and leave all of that FieldsWriter shifting alone. Being CLS compliant only applies to public and protected members. We can do whatever we want with internal or private members, including using non-CLS compliant types. On top of there being a lot of methods marked public that should be internal or protected internal, I think this might be easier than we originally thought.
          Hide
          Prescott Nasser added a comment -

          That's really kind of buried in the text of CLS compliance information - but once I started looking with that idea in mind it became clear pretty quick. Great find Chris.

          Show
          Prescott Nasser added a comment - That's really kind of buried in the text of CLS compliance information - but once I started looking with that idea in mind it became clear pretty quick. Great find Chris.
          Hide
          Christopher Currens added a comment -

          So, I've been using VS11 for all my development lately (it's ridiculously faster than VS2010). One of the great new features is that you can quickly and easily search through the Error List and find warnings you actually care about (we have 1700 warnings in Lucene.NET).

          If you do a search for CLS, you'll find we only have 53 violations! This is great news, because it means that we are getting ever closer to fixing this issue.

          However, here's the issue we need to address, that is not so simple. Of course, the main issue we have with CLS Compliance is having identifiers only differing in case. This is due in part to LUCENENET-468, where some properties now have the same name differing only in case with protected internal fields.

          Now, in many cases, this isn't an issue, since you can just turn it into an automatic property, where the setter is protected internal. However, my guts tells me it's not okay to do with properties that are virtual. So, how can we solve this? It seems we need to change the name of one of them, and prefixing the field with an underscore is not only the wrong naming style, it's actually not CLS compliant.

          Any thoughts?

          Show
          Christopher Currens added a comment - So, I've been using VS11 for all my development lately (it's ridiculously faster than VS2010). One of the great new features is that you can quickly and easily search through the Error List and find warnings you actually care about (we have 1700 warnings in Lucene.NET). If you do a search for CLS, you'll find we only have 53 violations! This is great news, because it means that we are getting ever closer to fixing this issue. However, here's the issue we need to address, that is not so simple. Of course, the main issue we have with CLS Compliance is having identifiers only differing in case. This is due in part to LUCENENET-468 , where some properties now have the same name differing only in case with protected internal fields. Now, in many cases, this isn't an issue, since you can just turn it into an automatic property, where the setter is protected internal. However, my guts tells me it's not okay to do with properties that are virtual. So, how can we solve this? It seems we need to change the name of one of them, and prefixing the field with an underscore is not only the wrong naming style, it's actually not CLS compliant. Any thoughts?
          Hide
          Prescott Nasser added a comment -

          I changed most of these to internalXXX with the latest commit. There were some that could be changed to auto-properties, but some that couldn't that applied a default value. We still have some CLS issues around volitile and sbyte, but we aren't going to solve those soon I think.

          Show
          Prescott Nasser added a comment - I changed most of these to internalXXX with the latest commit. There were some that could be changed to auto-properties, but some that couldn't that applied a default value. We still have some CLS issues around volitile and sbyte, but we aren't going to solve those soon I think.
          Hide
          michael herndon added a comment -

          That is probably fine for now. We still get a big win here as all your hard work so far should get Lucene.Net back to playing nice for VB.Net consumers.

          Show
          michael herndon added a comment - That is probably fine for now. We still get a big win here as all your hard work so far should get Lucene.Net back to playing nice for VB.Net consumers.

            People

            • Assignee:
              Prescott Nasser
              Reporter:
              Prescott Nasser
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:

                Development