C#: Refactor attribute extraction #4516

tamasvajk · 2020-10-20T06:18:46Z

No description provided.

tamasvajk · 2020-10-20T10:14:25Z

csharp/ql/test/library-tests/attributes/Assembly1.cs_

+    public class Class1
+    {
+        [CustomAttribute(42 + 0, new int[] { 1, 2, 3 }, typeof(Class1), Enum1.B, new int[] { 1, 2, 3 }, Prop2 = new object[] { 1, typeof(int) })]
+        [return: Custom(42 + 0, new int[] { 1, 2, 3 }, null, Enum1.A, new int[] { 1, 2, 3 }, Prop2 = new object[] { 1, typeof(int) })]


Note: we're not extracting this attribute.

tamasvajk · 2020-10-20T10:14:45Z

csharp/ql/test/library-tests/attributes/Assembly1.cs_

+using System;
+
+[assembly: Assembly1.CustomAttribute(1, new object[] { 1, 2, null }, typeof(Assembly1.CustomAttribute), (Assembly1.Enum1)12, null, Prop2 = new object[] { 1, typeof(int) })]
+[module: Assembly1.CustomAttribute(2, new object[] { 1, 2, null }, typeof(Assembly1.CustomAttribute), (Assembly1.Enum1)12, null, Prop2 = new object[] { 1, typeof(int) })]


We're extracting module: as if it was assembly:

csharp/ql/test/library-tests/attributes/DB-CHECK.expected

tamasvajk · 2020-10-20T12:56:28Z

https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/549/

tamasvajk · 2020-10-20T13:01:09Z

csharp/extractor/Semmle.Extraction.CSharp/Populators/CompilationUnit.cs

            var outputAssembly = Assembly.CreateOutputAssembly(cx);
+            var attributeDatas = cx.Compilation.Assembly.GetAttributes().ToList();
+            attributeDatas.AddRange(cx.Compilation.Assembly.Modules.SelectMany(m => m.GetAttributes()));
            foreach (var attribute in node.Attributes)
            {
-                var ae = new Attribute(cx, attribute, outputAssembly);
-                cx.BindComments(ae, attribute.GetLocation());
+                var attributeData = attributeDatas.Single(ad => ad.ApplicationSyntaxReference?.GetSyntax() == attribute);
+                if (attributeData is object)
+                {
+                    var ae = Attribute.Create(cx, attributeData, outputAssembly);
+                    cx.BindComments(ae, attribute.GetLocation());
+                }


This is quite ugly, but I couldn't find a better way to extract assembly attributes.

tamasvajk · 2020-10-21T06:34:54Z

https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/549/

There's no significant change in analysis times, maybe it got a bit slower.
No new real issues were found. There are changes in the obsolete-method-called check, but that might just be wobbliness. (There are true and false positives too.)
DB size increased by 0-3.5% depending on project.

tamasvajk · 2020-10-21T08:25:21Z

csharp/ql/src/semmle/code/csharp/PrintAst.qll

+    not isNotNeeded(element.getParent+()) and
+    // LambdaExpr is both a Callable and a ControlFlowElement,
+    // print it with the more specific CallableNode
+    not element instanceof Callable


This is unrelated to this PR, but it was consistently failing locally and in the CI job too, so I included the fix here.

tamasvajk · 2020-10-21T08:27:32Z

@hvitved one significant change by this PR is that we can have expressions outside of source files now. What do you think is this going to cause trouble somewhere?

tamasvajk · 2020-10-29T15:10:19Z

I've rebased this PR to fix merge conflicts.

Also starting a new c# differences job: https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/574/

csharp/ql/test/library-tests/controlflow/graph/ExitElement.expected

…rated expressions when missing

…when location is from source

- Only cache on `AttributeData` and not the parent entity. - Move `CreateGeneratedExpressionFromArgument` to `Expression.cs`. - Restructure the various `CreateGenerated` methods so child entities are created inside them (and therefore no need to expose child index logic). - Add locations to generated expressions. - Avoid linear lookup in `CompilationUnit.cs`. - Consolidate tests.

tamasvajk · 2020-11-11T09:00:15Z

@hvitved I rebased this PR, and pushed an extra commit to fix the failing tests, can you please double check if these were really expected elements?

I'm triggering a C# differences job in the meantime.

tamasvajk · 2020-11-11T09:02:48Z

Differences job

hvitved · 2020-11-11T12:52:18Z

@hvitved I rebased this PR, and pushed an extra commit to fix the failing tests, can you please double check if these were really expected elements?

Turns out that it is because the compiler inserts Nullable(1) attributes in tuple patterns, so we should be good.

hvitved

LGTM. Let's wait for the Differences job before merging.

github-actions bot added the C# label Oct 20, 2020

tamasvajk force-pushed the feature/attributes branch 3 times, most recently from 93c0351 to d211e4f Compare October 20, 2020 08:33

tamasvajk changed the title ~~Refactor attribute extraction~~ C#: Refactor attribute extraction Oct 20, 2020

tamasvajk force-pushed the feature/attributes branch from d211e4f to 38b95d4 Compare October 20, 2020 10:10

tamasvajk commented Oct 20, 2020

View reviewed changes

tamasvajk force-pushed the feature/attributes branch 3 times, most recently from 1a46cb9 to a10da96 Compare October 20, 2020 12:54

tamasvajk commented Oct 20, 2020

View reviewed changes

tamasvajk force-pushed the feature/attributes branch from bd2d6aa to 462cb48 Compare October 21, 2020 08:23

tamasvajk commented Oct 21, 2020

View reviewed changes

github-actions bot added the documentation label Oct 21, 2020

tamasvajk force-pushed the feature/attributes branch from 0676e06 to 8c4d5a1 Compare October 21, 2020 11:20

tamasvajk marked this pull request as ready for review October 21, 2020 12:31

tamasvajk requested a review from a team as a code owner October 21, 2020 12:31

tamasvajk dismissed a stale review via b61b6a2 October 29, 2020 15:07

tamasvajk force-pushed the feature/attributes branch from 8c4d5a1 to b61b6a2 Compare October 29, 2020 15:07

tamasvajk commented Oct 29, 2020

View reviewed changes

csharp/ql/test/library-tests/controlflow/graph/ExitElement.expected Outdated Show resolved Hide resolved

tamasvajk force-pushed the feature/attributes branch from cc37447 to 4d5aa22 Compare October 30, 2020 08:58

tamasvajk added 5 commits November 11, 2020 09:46

C#: Add missing tests for attribute extraction

c14a634

C#: Add attribute test for extraction from symbols without syntax nodes

883795e

C#: Change attribute extraction to use AttributeData and created gene…

c04d6f4

…rated expressions when missing

C#: Change extraction of Attributes to have ID based on the location …

cdea3af

…when location is from source

C#: Extract attributes of referenced assembly

5f18840

tamasvajk and others added 9 commits November 11, 2020 09:46

Fix failing tests

ba730b6

C#: Add change note for attribute extraction refactoring

4d6ccb1

C#: Fix failing tests

4b88b23

C#: Exclude assembly attribute expressions from CFG construction

91dd677

C#: Introduce AssertionFailure class

3048919

C#: Remove redundant conjunct

b8d6070

C#: Address comments

e57134e

Fix expected files of failing tests

453c97a

tamasvajk force-pushed the feature/attributes branch from a14beca to 453c97a Compare November 11, 2020 08:58

hvitved approved these changes Nov 11, 2020

View reviewed changes

tamasvajk merged commit b5ef3bd into github:main Nov 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

C#: Refactor attribute extraction #4516

C#: Refactor attribute extraction #4516

Uh oh!

tamasvajk commented Oct 20, 2020

Uh oh!

tamasvajk Oct 20, 2020

Uh oh!

tamasvajk Oct 20, 2020

Uh oh!

Uh oh!

tamasvajk commented Oct 20, 2020

Uh oh!

tamasvajk Oct 20, 2020

Uh oh!

tamasvajk commented Oct 21, 2020

Uh oh!

tamasvajk Oct 21, 2020

Uh oh!

tamasvajk commented Oct 21, 2020

Uh oh!

tamasvajk commented Oct 29, 2020 •

edited

Loading

Uh oh!

Uh oh!

tamasvajk commented Nov 11, 2020

Uh oh!

tamasvajk commented Nov 11, 2020

Uh oh!

hvitved commented Nov 11, 2020

Uh oh!

hvitved left a comment

Uh oh!

Uh oh!

C#: Refactor attribute extraction #4516

C#: Refactor attribute extraction #4516

Uh oh!

Conversation

tamasvajk commented Oct 20, 2020

Uh oh!

tamasvajk Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

tamasvajk Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tamasvajk commented Oct 20, 2020

Uh oh!

tamasvajk Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

tamasvajk commented Oct 21, 2020

Uh oh!

tamasvajk Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

tamasvajk commented Oct 21, 2020

Uh oh!

tamasvajk commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tamasvajk commented Nov 11, 2020

Uh oh!

tamasvajk commented Nov 11, 2020

Uh oh!

hvitved commented Nov 11, 2020

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tamasvajk commented Oct 29, 2020 •

edited

Loading