Null-ref-exceptions experienced when run against NHibernate project

[sbohlen]

Docu Team:

As promised, here is a run-down of the changes I found I needed to
make to the Docu source in order to resolve several unhandled
exceptions experienced when testing Docu against the NHibernate
project.

As already mentioned elsewhere, I'm offering this *not* for the value
of the fixes applied (since I doubt they are 'correct' for what you
want Docu to do) but for the (potential) value in their pointing out
places within Docu that might need to be provided some safety-net in
order to process code comments from other projects in the future.
Feel free to disregard this thread as you see fit -- take it or leave
it :)

My primary error from which most of the others arose was in the src
\Docu.Console\Parsing\Comments\SeeCodeCommentParser.cs

Line 12 reads...

var referenceTarget = Identifier.FromString(content.Attributes
["cref"].Value);

...and I was getting a null-reference exception when attempting to
access the .Value property on the contents.Attributes["cref"] element.

The quick (and completely hacky) solution I came up with was to adjust
the code preceding the large if-then-else block in the rest of the
method as follows...

IReferencable reference = null;

if (content.Attributes["cref"] == null)
return new See(reference);

var referenceTarget = Identifier.FromString
(content.Attributes["cref"].Value);

//rest of method here...

...so that it just returns a new See(null) before processing the rest
of the SeeCodeCommentParser.Parse(...) method. While this worked for
stopping my error(s), its probably *not* the desired behavior of Docu
so I'm not suggesting you accept this change at all of course :)

This change in the SeeCodeCommentParser.Parse(...) method meant that
it was then entirely possible that references could be null in other
parts of the codebase and so the following snippet of code that
appears in several different classes elsewhere (typically the 'Resolve
(...)' method of specifically the Event, Field, Method, Property, and
DeclaredType classes)...

foreach (IReferrer comment in Summary.Where(x => x is
IReferrer))
{
if (!comment.Reference.IsResolved)
comment.Reference.Resolve(referencables);
}

...needed to be changed slightly to include a null-check in the if
conditional as in...

foreach (IReferrer comment in Summary.Where(x => x is
IReferrer))
{
if ((comment.Reference != null) && (!
comment.Reference.IsResolved))
comment.Reference.Resolve(referencables);
}

Once this change was applied to all of those classes listed above, my
next issue arose out of some troubles with the PropertyGenerator and
MethodGenerator classes.

For both of these classes, I needed to modify the final lines of the
Add(...) method to check the 'type' variable for null and bail out if
needed as in...

matchedAssociations.Add(association.Name, doc);
if (type == null) return;
type.AddMethod(doc);

...instead of just...

matchedAssociations.Add(association.Name, doc);
type.AddMethod(doc);

My final needed edit was to the PropertyGenerator.Add(...) method. I
found that I needed to add the test...

if (matchedAssociations.ContainsKey(association.Name))
return;

...to check for duplicate keys in the matchedAssociations collection
else I was getting an exception at this point too. Interestingly,
there was actually already a nearly 100% identical line in the
'sister' class, MethodGenerator, that contained exactly this very same
check code along with a comment that explains what's going on...

if (matchedAssociations.ContainsKey(association.Name))
return; // weird case when a type has the same method
declared twice

...so fixing the PropertyGenerator.Add(...) method was literally a
matter of just adding the same check that someone on the team had
already added to the MethodGenerator.Add(...) method to solve the same
issue with that class' collection. I agree with the comment that the
condition is indeed weird as it seems like it shouldn't be permitted
to exist in the source code of the assembly being analyzed, but I have
to say that whatever causes this condition pretty clearly exists in
the NHibernate source *somewhere* :D

In any case, I hope this thread helps you guys nail down the *correct*
changes to the source to resolve whatever these issues are in the way
that most makes sense for Docu; as I said, these simple edits and
hacks got my build of Docu to the point where it could reasonably
compile the NH XML code comments without any exceptions and that's all
I sort of needed for my near-term evaluation.

Hope this helps somewhat!

-Steve B.

[James Gregory]

Stephen: I've just run NHibernate through Docu and got the null references as you did and I now know why it's happening.

NHibernate uses some NDoc proprietary comments that Docu doesn't gracefully handle. Specifically the xml comment spec dictates that a <see /> element must have a cref attribute, but ndoc introduced an alternative langword attribute for referring to C# keywords. Docu didn't expect this and fails miserably. Still, it raises an interesting point... do we support NDoc keywords?

You can see the NDoc comments here, some seem more useful than others. The langword attribute is nice, so you can refer to null and public and such, but things like overloads seems a little pointless.

On Tue, Mar 31, 2009 at 4:26 AM, Jason Meridth <jmer...@gmail.com> wrote:

I've added your changes and sent pull request to James.

No tests though.  *slaps self in forehead really hard*

Going to try and add them now.

I am still ramping up on the project.
---
Jason Meridth
http://jason.lostechies.com

[Stephen Bohlen]

Interesting observation.

I would have to guess that the NDoc-style comments are an artifact left over from a time when NDoc was the preferred (read: only!) method of compiling XML code comments.  Since NH has been around for some time, these 'non-standard' comment syntaxes obviously must be remnants in the NH codebase from long ago (well, long ago in software terms at least!).

I would sort of think that there are two related but also orthogonal issues to consider here: 1) should Docu support NDoc custom code comment extensions as you properly raise the issue of? and 2) how should Docu be extended to handle conditions where comment sytaxes are found to be 'invalid' as Docu understands them?  

I'd guess that even if the Docu team were to decide to support NDoc comments and custom keywords, you'd still have the condition where someone typed a non-parsable keyword into their comments (e.g., <steve>some value here</steve>, <steve topic="hello">something</steve>   etc.) and of course have to deal with that too (even if you just swallow the 'error' and ignore it and move on).

My own 2-cents is that supporting NDoc custom code comment syntaxes probably isn't a great idea as such things are really 'legacy' artifacts at this point in the timeline of the evolution of code comment syntaxes and NDoc of course is dead now (and probably not going to come back to life in either of our lifetimes).  But the degree of reverse-compatibility that you may want Docu to support (e.g., older codebases, a migration path for past NDoc users, whatever) is something the Docu team will need to decide on the relative value of based on its own merits, of course.

In any event, I'm glad you were able to determine the cause of the exceptions I was experiencing -- that's one less mystery in my life~!

-Steve B.

--
Steve Bohlen
sbo...@gmail.com
http://blog.unhandled-exceptions.com
http://twitter.com/sbohlen

[junior]

I also ran into similar issues, I have made a patch for handling <see
href=""/> and <see langword=""/> elements for my project. Ive uploaded
it as a file for the group, in case anyone else might find it useful :)

[Jamie Penney]

Is it possible that the duplicate methods were the declaration and
implementation of a partial method?

[James Gregory]

I think there's a lot of refining that needs to be done in the assembly and xml parsing code before we can squash out the duplication issues. In my local copy (not pushed to github yet) I've committed a change that effectively turns on warnings for duplication issues, and there's a helluva lot of warnings that get printed; I definitely think there's some extra calls in there that shouldn't be.

I was hoping to resolve this issue before pushing my changes, but if anybody is interested (and doesn't mind seeing hundreds of warnings when they build their docs) then I can push it to github.

[junior]

Jamie, I have just run into a duplicate key issue also, and the
affected classes are part of visual studio generated Web References -
I have 2 partial classes named the same but are in different
namespaces (in differnet web references) and they appear to be
resolving to the same namespace at this line:

matchedAssociations.Add(association.Name.CloneAsNamespace(), doc); //
throws 'item with same key already added' exception

On 2 May, 21:38, Jamie Penney <ja...@jamiepenney.co.nz> wrote:
> Is it possible that theduplicatemethods were the declaration and

> > ...to check forduplicatekeys in the matchedAssociations collection

> > else I was getting an exception at this point too.  Interestingly,
> > there was actually already a nearly 100% identical line in the
> > 'sister' class, MethodGenerator, that contained exactly this very same
> > check code along with a comment that explains what's going on...
>
> >             if (matchedAssociations.ContainsKey(association.Name))
> >                 return; // weird case when a type has the same method
> > declared twice
>
> > ...so fixing the PropertyGenerator.Add(...) method was literally a
> > matter of just adding the same check that someone on the team had
> > already added to the MethodGenerator.Add(...) method to solve the same
> > issue with that class' collection.  I agree with the comment that the
> > condition is indeed weird as it seems like it shouldn't be permitted
> > to exist in the source code of the assembly being analyzed, but I have
> > to say that whatever causes this condition pretty clearly exists in
> > the NHibernate source *somewhere* :D
>
> > In any case, I hope this thread helps you guys nail down the *correct*
> > changes to the source to resolve whatever these issues are in the way
> > that most makes sense for Docu; as I said, these simple edits and
> > hacks got my build of Docu to the point where it could reasonably
> > compile the NH XML code comments without any exceptions and that's all
> > I sort of needed for my near-term evaluation.
>
> > Hope this helps somewhat!
>

> > -Steve B.- Hide quoted text -
>
> - Show quoted text -

[junior]

I would be interested in using your change to turn on warnings for
duplication issues, if you can make a patch or branch available that
would be great.