All languages: Use shared FileSystem library and minor regex performance improvement. by aschackmull · Pull Request #14321 · github/codeql · GitHub

aschackmull · 2023-09-26T12:10:58Z

This switches C++, Go, Java, JavaScript, Python, and QL to the shared FileSystem library as a followup to #12289.

I've also made a small performance tweak to the regex matching that extracts base name, stem, and extension from absolute file paths by reusing the same regex instead of calculating the same match 3 times.

Fixes https://github.com/github/codeql-c-team/issues/1598 and https://github.com/github/codeql-go-team/issues/325

owen-mc

Go 👍🏻

hvitved · 2023-09-26T13:01:23Z

shared/util/codeql/util/FileSystem.qll

     * Gets the absolute path of this container.
     *
-     * Typically `containerparent(result, this)`.
+     * Typically `folders(this, result) or files(this, result)`.


Now that we are touching this file anyway, could we make the three user-facing classes Container, Folder, and File final through final aliases, make the existing classes private and add an Impl suffix to the name, and make ContainerImpl and ContainerImpl::getUrl abstract?

That causes the need for all sorts of cascading changes throughout the Java class hierarchy. And maybe also for the some of the other languages. So I'm not going to try to fit that into this PR.

yoff

Looks fine from the Python side. Just a few questions to make sure I understand the refactor.

yoff · 2023-09-27T08:33:51Z

python/ql/lib/semmle/python/Files.qll

-    )
-  }
+class Container extends Impl::Container {
+  Container getParent() { result = this.getParentContainer() }


Looks like we have an extra class predicate here that we should clean up (users should just use getParentContainer instead)?

Yeah, I didn't want to change things so I just left it in.

That is fine, we can clean it up later :-)

yoff · 2023-09-27T08:34:10Z

python/ql/lib/semmle/python/Files.qll

-  }
-
-  Container getParentContainer() { this = result.getAChildContainer() }
+  override Container getParentContainer() { result = super.getParentContainer() }


This is needed in order to cast the result?

Yes. It's used elsewhere to implement a signature so the exact return type is needed.

jketema

C++ 👍

yoff · 2023-09-27T09:32:51Z

C++ 👍

Looks like C++ might need updated test expectations.

jketema · 2023-09-27T09:40:35Z

C++ 👍

Looks like C++ might need updated test expectations.

Mmm, I must have mis-clicked when looking at the test regressions. I didn't see that. I'd actually like to know why this changes. This seems somewhat suspect.

aschackmull · 2023-09-27T09:52:14Z

C++ 👍

Looks like C++ might need updated test expectations.

Mmm, I must have mis-clicked when looking at the test regressions. I didn't see that. I'd actually like to know why this changes. This seems somewhat suspect.

Previously C++ didn't have a getURL override, but rather used getLocation for these classes. Now it inherits getURL from the shared library, which takes precedence over getLocation, so the url of the folder / ends up as folder:/// instead of file://:0:0:0:0. I'm not sure what's preferable. At least the updated behaviour will match the other languages and it seems that the previous behaviour was C++ being the odd one out.

jketema · 2023-09-27T09:56:37Z

C++ 👍

Looks like C++ might need updated test expectations.

Mmm, I must have mis-clicked when looking at the test regressions. I didn't see that. I'd actually like to know why this changes. This seems somewhat suspect.

Previously C++ didn't have a getURL override, but rather used getLocation for these classes. Now it inherits getURL from the shared library, which takes precedence over getLocation, so the url of the folder / ends up as folder:/// instead of file://:0:0:0:0. I'm not sure what's preferable. At least the updated behaviour will match the other languages and it seems that the previous behaviour was C++ being the odd one out.

Looking at the test, I think folder:/// is actually more correct. However, the getURL we had was also removed purposefully in 5fdfd98

aschackmull · 2023-09-27T11:29:36Z

However, the getURL we had was also removed purposefully in 5fdfd98

That's not the sha in which it was deprecated, though, that predates the move-to-public. But one of the very first PRs in this repo is relevant in this context: #3

aschackmull · 2023-09-27T11:31:13Z

Maybe Container simply shouldn't be Locatable.

jketema · 2023-09-27T11:56:38Z

However, the getURL we had was also removed purposefully in 5fdfd98

That's not the sha in which it was deprecated, though, that predates the move-to-public.

Yup.

So, I'm happy with just accepting the test change. I do think we maybe should consider deprecating/removing getURL wholesale.

aschackmull · 2023-09-27T12:38:27Z

I do think we maybe should consider deprecating/removing getURL wholesale.

Or the opposite. Remove Folder from Locatable. Currently C++ is the odd one out here, all other languages don't have locations for folders.

jketema · 2023-09-27T13:40:05Z

I do think we maybe should consider deprecating/removing getURL wholesale.

Or the opposite. Remove Folder from Locatable. Currently C++ is the odd one out here, all other languages don't have locations for folders.

That also makes sense. I don't have any particular preference here.

aschackmull · 2023-09-28T07:03:15Z

That also makes sense. I don't have any particular preference here.

In any case, I think that's best handled as follow-up.

jketema · 2023-09-28T07:12:53Z

That also makes sense. I don't have any particular preference here.

In any case, I think that's best handled as follow-up.

Agreed.

aschackmull added the no-change-note-required This PR does not need a change note label Sep 26, 2023

aschackmull requested review from a team as code owners September 26, 2023 12:10

github-actions bot added C# JS C++ Java Python Go Ruby QL-for-QL labels Sep 26, 2023

owen-mc approved these changes Sep 26, 2023

View reviewed changes

hvitved reviewed Sep 26, 2023

View reviewed changes

yoff approved these changes Sep 27, 2023

View reviewed changes

jketema approved these changes Sep 27, 2023

View reviewed changes

aschackmull added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Sep 27, 2023

aschackmull added 10 commits September 28, 2023 08:58

Java: Use shared FileSystem library.

653844c

C++: Use shared FileSystem library.

2548939

Go: Use shared FileSystem library.

a08fe5b

Python: Use shared FileSystem library.

73521ca

QL: Use shared FileSystem library.

7bc0c54

JavaScript: Use shared FileSystem library.

855c896

Ruby: Minor simplification.

68d05eb

C#: Minor simplification.

20cbab9

FileSystem: Fix qldoc.

cfd08f2

FileSystem: Improve regex by only matching once instead of 3x.

80f00bc

aschackmull force-pushed the shared/filesystem branch from 721cdc0 to 80f00bc Compare September 28, 2023 06:59

aschackmull merged commit 5feb2f7 into github:main Sep 28, 2023

aschackmull deleted the shared/filesystem branch September 28, 2023 12:55

aschackmull mentioned this pull request Sep 28, 2023

C++: Remove getLocation from Container. #14337

Merged

Conversation

aschackmull commented Sep 26, 2023

Uh oh!

owen-mc left a comment

Choose a reason for hiding this comment

Uh oh!

hvitved Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

aschackmull Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

yoff Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

aschackmull Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

yoff Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

yoff Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

aschackmull Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

jketema left a comment

Choose a reason for hiding this comment

Uh oh!

yoff commented Sep 27, 2023

Uh oh!

jketema commented Sep 27, 2023

Uh oh!

aschackmull commented Sep 27, 2023

Uh oh!

jketema commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aschackmull commented Sep 27, 2023

Uh oh!

aschackmull commented Sep 27, 2023

Uh oh!

jketema commented Sep 27, 2023

Uh oh!

aschackmull commented Sep 27, 2023

Uh oh!

jketema commented Sep 27, 2023

Uh oh!

aschackmull commented Sep 28, 2023

Uh oh!

jketema commented Sep 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jketema commented Sep 27, 2023 •

edited

Loading