Change ASTUnit::getASTContext() const to return a non-const ASTContext

[bricknerb]

Also, remove the non-const ASTUnit::getASTContext() since it's no longer necessary.
This would make it similar to Decl::getAstContext() const for a more consistent and flexible API.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Boaz Brickner (bricknerb)

Changes

Also, remove the non-const ASTUnit::getASTContext() since it's no longer necessary.
This would make it similar to Decl::getAstContext() const for a more consistent and flexible API.

---

Full diff: https://github.com/llvm/llvm-project/pull/130096.diff

1 Files Affected:

• (modified) clang/include/clang/Frontend/ASTUnit.h (+1-2)

diff --git a/clang/include/clang/Frontend/ASTUnit.h b/clang/include/clang/Frontend/ASTUnit.h
index 1f98c6ab328ba..bd55c8b627941 100644
--- a/clang/include/clang/Frontend/ASTUnit.h
+++ b/clang/include/clang/Frontend/ASTUnit.h
@@ -439,8 +439,7 @@ class ASTUnit {
   Preprocessor &getPreprocessor() { return *PP; }
   std::shared_ptr<Preprocessor> getPreprocessorPtr() const { return PP; }
 
-  const ASTContext &getASTContext() const { return *Ctx; }
-  ASTContext &getASTContext() { return *Ctx; }
+  ASTContext &getASTContext() const { return *Ctx; }
 
   void setASTContext(ASTContext *ctx) { Ctx = ctx; }
   void setPreprocessor(std::shared_ptr<Preprocessor> pp);

[AaronBallman]

This is weakening const correctness, which is the opposite direction of where we should be going IMO.

[Endilll]

I agree with Aaron that this patch doesn't move us towards the direction we generally want. I'd like to see more justification than 2 lines in the description.

[mizvekov]

There is no const_cast, or casting away const in any form on this patch, so how does it weaken const correctness?

[mizvekov]

To put it another way, what do we want to prevent the user from accomplishing if he obtains a non-const reference to ASTContext from a const ASTUnit?

[AaronBallman]

There is no const_cast, or casting away const in any form on this patch, so how does it weaken const correctness?

When a function is marked as const, the implication is that calling the function should not lead to an underlying mutation. Given that this function is returning data it holds (*Ctx), the expectation is that the return value is also const-qualified. The typical pattern is to have an overload set as in the original code -- the non-const version can return something which can mutate, the const version can return something which doesn't allow for mutation without casting away the constness.

[bricknerb]

Thanks for the feedback!

Is there a way to call ASTContext::createMangleContext() when I have a const ASTUnit?
I've tried to make ASTContext::createMangleContext() const but this failed as it seems to actually call methods that potentially modify the context.

However, when I have a const Decl, I can just call getAstContext().createMangleContext().

[AaronBallman]

Thanks for the feedback!

Is there a way to call ASTContext::createMangleContext() when I have a const ASTUnit? I've tried to make ASTContext::createMangleContext() const but this failed as it seems to actually call methods that potentially modify the context.

However, when I have a const Decl, I can just call getAstContext().createMangleContext().

There's two answers. In a world where we have const correctness, you shouldn't be able to (this is a misfeature on Decl that it has a const overload that returns a const ASTContext &). However, Clang inherited a lack of const correctness from the mists of time, so we have these sort of misfeatures still lurking (and will for quite some time). So for now, the recommendation is const_cast so it's clear where we're breaking const correctness.

[shafik]

Just throwing in my $0.02 and saying I agree w/ Aaron and Vlad here, this is not the right direction. Having a const method return a non-const that we will then mutate is just not clean code.

Having to use const_cast documents we are violating expectations and marks this as something we need to clean up in the future.

[bricknerb]

Thanks everyone!
Closing this.