C#: Reduce size of getAThrownException()

[hvitved]

In the precense of multiple core libraries, getAThrownException() would return
multiple copies of the same exception, say System.OverflowException, one for each
core library. With this change we try to identify which core library a given control
flow element was compiled against, and only return the corresponding version.

[hvitved]

@calumgrant : Can you think of a better way to do this?

[hvitved]

Performance unchanged by this change (internal link): https://git.semmle.com/gist/tom/09129553dee65e843c2376651987ce33.

[calumgrant]

I have 2 other ideas, which may well prove to be worse than this, but I thought I would put them here.

1. Canonicalise the exception types, so System.NullReferenceException would always refer to a single entity, picking them according to some priority. So you could get a method throwing the wrong type but I don't think that will actually matter in practise.
2. Instead of looking at all type mentions, only look at attributes. Whilst object is canonical and cannot be used, most assemblies will use attributes, so look at the defining assembly of System.Attribute.

[hvitved]

• Canonicalise the exception types, so System.NullReferenceException would always refer to a single entity, picking them according to some priority. So you could get a method throwing the wrong type but I don't think that will actually matter in practise.

This would require an extractor change, and it seems to me a bit ad hoc to treat exception types specially.

• Instead of looking at all type mentions, only look at attributes. Whilst object is canonical and cannot be used, most assemblies will use attributes, so look at the defining assembly of System.Attribute.

That might work, but would you prefer that for better performance of higher precision?

[calumgrant]

• Canonicalise the exception types, so System.NullReferenceException would always refer to a single entity, picking them according to some priority. So you could get a method throwing the wrong type but I don't think that will actually matter in practise.

This would require an extractor change, and it seems to me a bit ad hoc to treat exception types specially.

Not necessarily. You could use QL to create something like

class CanonicalType extends ValueOrRefType {
  CanonicalType() {
    not exists(ValueOrRefType t | this.getQualifiedName() = t.getQualifiedName() and
      t.getLocation().(Assembly).getName() > this.getLocation().(Assembly).getName()
    )
  }
}

Class getAThrownException() {
  result instanceof CanonicalType and
  ...
}

• Instead of looking at all type mentions, only look at attributes. Whilst object is canonical and cannot be used, most assemblies will use attributes, so look at the defining assembly of System.Attribute.

That might work, but would you prefer that for better performance of higher precision?

It might be a little simpler and faster. Don't know.

[hvitved]

Not necessarily. You could use QL to create something like

Makes sense. My only concern is that we would then also have to canonicalize explicitly thrown exceptions, throw new Exception(), where we can now just use the type produced directly by the extractor.

[calumgrant]

Not necessarily. You could use QL to create something like

Makes sense. My only concern is that we would then also have to canonicalize explicitly thrown exceptions, throw new Exception(), where we can now just use the type produced directly by the extractor.

Yes, getAThrownException() for throw expressions would need special treatment.

I wonder if it could be the case that a library throws netstandard::System.Exception, and another module catches corlib::System.Exception. In that case, you'd probably want to match up the two types anyway. I'm not sure how often this occurs in practice.

[calumgrant]

But possibly, CanonicalType might be a useful concept to use here.

[calumgrant]

Approach LGTM. I guess this is quite hard to qltest, but I'd be interested in seeing the effect on performance.

[calumgrant]

One thing that might go a bit wrong here is that you'll end up querying ObjectType.getALocation() which will in fact return all CoreLibs. So perhaps

result = scc.getCaughtExceptionType().getBaseClass*().(SystemExceptionClass).getALocation()

[calumgrant]

Also getBaseClass

[hvitved]

Yes, that's a clear oversight, good spot!

[calumgrant]

core = a.getAnAttribute().getType().getBaseClass*().(SystemAttributeClass).getALocation()

otherwise we'll end up getting the location of ObjectType which will be all of them.

Also note getBaseClass() as we don't care about interfaces.

[calumgrant]

I just remembered that only source assemblies have attributes (due to extractor limitations). I don't think this affects anything here but is worth bearing in mind.

[hvitved]

Again, good spot :-)