dyno: Make some improvements to const checking for associated actions #24734
Conversation
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
It's very important that such determinations about whether or not to add a default function be made with only information available in the module containing the type. The production compiler does it with some hacky ways to check for methods with certain names. Arguably, doing a regular resolution check is an improvement upon that. But either way, we can't have it do something like depend on point-of-instantiation. Anyway, another option here to avoid the cycle you describe is to do the call resolution for a test call but ignore compiler-generated candidates when doing so. That doesn't seem too far afield to me because we'll also want to ignore POI while doing this check. |
| Determine if a type is mutated when it is copied. For example, any record | ||
| containing an 'owned' class field is mutated when copied using a | ||
| default copy-initializer. Any type attributed with 'PRAGMA_COPY_MUTATES' | ||
| is considered to be mutated on copy. |
There was a problem hiding this comment.
IMO it would be nice for this comment to mention init= somewhere
| const uast::Decl* formalAst = nullptr; | ||
| bool typeHasDefault = isTypeWithDefaultValue(context, fieldQt.type()); | ||
| bool formalHasDefault = rf.fieldHasDefaultValue(i) || typeHasDefault; |
There was a problem hiding this comment.
put typeHasDefault first in the || since we just computed it, so perhaps we don't have to use fieldHasDefaultvalue ?
| const PoiScope* poiScope) { | ||
| static const TypedFnSignature* | ||
| tryResolveZeroArgMethod(Context* context, UniqueString name, | ||
| const AstNode* astForScopeOrErr, |
There was a problem hiding this comment.
To make the query using this more reusable, let's change astForScopeOrErr into an ID since that is all that is needed within
| // TODO: Skipping avoids a recursive query but doesn't handle | ||
| // mutually recursive classes. | ||
| if (ft == t) continue; | ||
| const uast::AstNode* ast = nullptr; |
There was a problem hiding this comment.
see my above comment -- I'd like us to remove this variable / the use of idToAst.
There was a problem hiding this comment.
Should I plumb this id only all the way down into resolveGeneratedCall or just stop in the helpers?
There was a problem hiding this comment.
resolveGeneratedCall only uses it for resolveFnCallSpecial, and there I think the only queries using it as an argument are ones that only run when we encounter an error. As a result, I don't think it's important to change resolveGeneratedCall in this way right now (but I would want this change if one day it became a query).
| if (t == CompositeType::getRangeType(context)) return true; | ||
| if (t == CompositeType::getLocaleType(context)) return true; | ||
| if (t->isPrimitiveType()) return true; | ||
| if (getTypeGenericity(context, t) == Type::GENERIC) return false; |
There was a problem hiding this comment.
I suggest swapping this line with the previous, since something like integral might be considered a primitive type, but can't be default initialized
| return kind() == QualifiedType::CONST_REF; | ||
| } | ||
| bool isRefMaybeConst() const { | ||
| return kind() == QualifiedType::REF_MAYBE_CONST; |
There was a problem hiding this comment.
IIRC @jabraham17 has removed ref-maybe-const formals from the production compiler. I don't recall if they have gone through a whole deprecation cycle. But we should probably remove this logic from dyno. Could you make an issue about it?
There was a problem hiding this comment.
| How do I know if it's assigning from or assigning to? | ||
| bar(foo()) | ||
| bar could have in intent -> assign to formal from foo() | ||
| bar could have out intent -> assign from formal to foo() |
There was a problem hiding this comment.
What was the solution to this conundrum in the end? Are we leaving it as future work or did you finesse the issue by assuming it's whatever would not have been a const-ness error?
There was a problem hiding this comment.
As far as const checking goes, it seems like if foo() returns by const ref then it cannot be passed to bar in the first place. I get a const checking error earlier in the pass before the associated action is even checked.
I'm blanking here - is there another pattern I could test here to make sure out is doing the right thing?
There was a problem hiding this comment.
I don't think further action is needed here right now & I'm happy with your answer to my question.
This PR introduces some const checking for associated actions such as assignment or init copy.
To help facilitate the changes, it adjusts the
Accesstype inmaybe-const.cppso that it is now a wrapper around aQualifiedType. The enum values have been changed into methods. TheAccesstype now provides an abstracted view whereisRefdoes not necessarily implyisConstRef, andisConstapplies to values as well (previously, the code only cared about constness for "ref" entities, and values were entirely ignored).Rename
isTypeDefaultInitializabletoisTypeWithDefaultValue, as this is more abstract, and not all types are necessarily types with a "default initializer".Add
Type::isMutatedOnCopyandType::isMutatedOnAssignment. These determine whether or not default-generatedinit=and=need to take the RHS byrefintent.Add a new test file
testTypeProperties.cppwhich can be used to test things likeisTypeWithDefaultValue.FUTURE WORK
compilerError'd initializers, andWHERE_TBDinisTypeWithDefaultValue.isDefaultInitializableCompositeas a separate user-facing query, as we may need to determine if a class type can be default initialized when evaluating anewexpression.isTypeWithDefaultValue...... One option here could be to havecanDefaultInitCompositeQuerynot rely on regular call resolution when searching for candidates.