Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++: Add getTemplateClass to DeductionGuide #17118

Merged
merged 2 commits into from
Aug 2, 2024
Merged

Conversation

jketema
Copy link
Contributor

@jketema jketema commented Aug 1, 2024

Drive-by fix in this PR: give an older change note a better name.

@jketema jketema added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Aug 1, 2024
@jketema jketema marked this pull request as ready for review August 1, 2024 14:39
@jketema jketema requested a review from a team as a code owner August 1, 2024 14:39
C(const T);
};

C(const double) -> C<int>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't we finding this? It's the one that should be selected by new C(0.0);, so it's not even unused, which would have been my best guess.

Are we missing the deduction guide itself, or its template class? I.e. can we add a separate query that just does from DeductionGuide d select d?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are finding it, but the frontend doesn't assign a proper location to it. This is the following line in the expected test results:

| file://:0:0:0:0 | C | test.cpp:4:8:4:8 | C<T> |

Copy link
Contributor

@sashabu sashabu Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was assuming that's the copy deduction candidate (which indeed wouldn't have a location)?

In any case, an additional fictional function template derived as above from a hypothetical constructor C(C) is added, called the copy deduction candidate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I guess an easy way to check would be to remove C(const double) -> C<int>; and see if | file://:0:0:0:0 | C | test.cpp:4:8:4:8 | C<T> | is still there.

Copy link
Contributor Author

@jketema jketema Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copy deduction one will be the unnamed one, which has the location of the class definition.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 But the unnamed one does have a location, and it's on line 5, where the C(const T); constructor is?

Basically, we have 4 deduction guides here and only 3 results lines (copy deduction guide, the guide for the C(const T); constructor, and the two explicit guides).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the copy one is likely missing. Can we side-step the discussion about missing ones for now, and open an internal issue for this? This is pretty much out-of-scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I agree based on the new test results. Could you please report:

  1. The missing locations for non-template deduction guides
  2. The missing copy deduction guide?

@@ -0,0 +1,18 @@
// semmle-extractor-options: -std=c++20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see some more tests:

  1. If we don't use CTAD, are (implicit) deduction guides still emitted?
  2. If we have multiple implicit guides, are they all emitted, or only the ones that are used?
  3. If we have multiple explicit guides, are they all emitted, or only the ones that are used?

For 2+3, I think we can just extend the existing test to add an unused two-argument constructor and an unused two-argument guide. 1 would need to be a separate test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. If we don't use CTAD, are (implicit) deduction guides still emitted?

I don't believe so, because that would have shown up in many other tests already, and it generally doesn't. Note that this is not related to what I'm doing here, as I'm only adding a relation between deduction guides and the class templates. Not adding more deduction guides to the database.

  1. If we have multiple implicit guides, are they all emitted, or only the ones that are used?
  2. If we have multiple explicit guides, are they all emitted, or only the ones that are used?

Would you be able to spell out some examples? That's probably the fastest way to get this done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take the test added in this PR and add:
2. A constructor C(int, int)
3. A guide C(char, char) -> C<char>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added 2 & 3.

import cpp

from DeductionGuide d
select d, d.getTemplateClass()
Copy link
Contributor

@sashabu sashabu Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this to:

from DeductionGuide d, string tc
where if exists(d.getTemplateClass()) then tc = d.getTemplateClass().toString() else tc = "<none>"
select d, tc

I think that would help clarify whether various deduction guides are themselves missing, or they don't have a template class associated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a second query instead of changing the existing one.

C(const T);
};

C(const double) -> C<int>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I agree based on the new test results. Could you please report:

  1. The missing locations for non-template deduction guides
  2. The missing copy deduction guide?

@jketema jketema merged commit 12261e6 into github:main Aug 2, 2024
16 of 17 checks passed
@jketema jketema deleted the ctad branch August 2, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants