[webkit-dev] Set once pointer types and static analysis

[Ryosuke Niwa]

RetainPtr isn’t supported by static analyzer at the moment.

> On Nov 13, 2024, at 4:28 AM, Jean-Yves Avenard <jean-yves.avenard at apple.com> wrote:
> 
> Hi
> 
> That sounds great.
> 
> Can we also do the same for const RetainPtr ?
> 
> Thanks
> 
> 
>> On 13 Nov 2024, at 4:49 PM, Ryosuke Niwa via webkit-dev <webkit-dev at lists.webkit.org> wrote:
>> 
>> Instead of introducing a new smart pointer type like SetOnceRefPtr, we’re going to use `const RefPtr`, `const Ref`, `const unique_ptr`, and `const UniqueRef`. `const Ref` and `const UniqueRef` are used for values that are initialized in the constructor and never changed. `const RefPtr` and `const unique_ptr` are used for lazily initialized values. We introduce a `initializeOnce(ptr, v)` function which takes `const RefPtr` or `const unique_ptr` ptr and sets it to v. The static analyzer will be updated to recognize this pattern.
>> 
>> https://bugs.webkit.org/show_bug.cgi?id=283038
>> https://github.com/llvm/llvm-project/pull/115594
>> 
>> - R. Niwa
>> 
>>> On Oct 29, 2024, at 2:49 AM, youenn fablet <youennf at gmail.com> wrote:
>>> 
>>> FWIW, there are many classes with Ref<> members that are initialized in the constructor and are never intended to be changed.
>>> I was hoping we would cover this case at least. The RefPtr lazy initialization is a welcome addition as well.
>>> 
>>> I think this would be useful to improve readability:
>>> - Too many protected in the same line of code makes code harder to read for me.
>>> - It is easier to reason about "stable" member variables. Having an explicit type or some annotation instead of having to look at the whole code is an improvement.
>>> 
>>> Le mar. 29 oct. 2024 à 03:10, Jean-Yves Avenard via webkit-dev <webkit-dev at lists.webkit.org> a écrit :
>>>> Hi
>>>> 
>>>> It’s a very strong +1 for me ; I find the usage of foo->protectedBar()->method() or Ref { foo->bar() }->method()
>>>> particularly unsightly (and points more to the inability of the static analyser to deal with some use case than the code being inherently unsafe)
>>>> 
>>>> Having a way to reduce the unsightly pattern where we are 100% certain it’s not needed is a benefit.
>>>> I assume also that the static analyser will not complain with `const` members either 
>>>> 
>>>> (For the sake of disclosure, I asked Ryosuke for that feature after discussing the need with Youenn) 
>>>> 
>>>> Jean-Yves
>>>> 
>>>>> On 29 Oct 2024, at 5:48 am, Ryosuke Niwa via webkit-dev <webkit-dev at lists.webkit.org> wrote:
>>>>> 
>>>>> Hi all,
>>>>> 
>>>>> In WebKit, it’s fairly common to write a member variable as RefPtr or std::unique_ptr that later gets lazily initialized to some value but never unset or assigned of a different value after that.
>>>>> 
>>>>> e.g.
>>>>> 
>>>>> class Foo {
>>>>>  Bar& bar() {
>>>>>      if (!m_bar)
>>>>>          m_bar = Bar::create();
>>>>>      return *m_bar;
>>>>>  }
>>>>>  Ref<Bar> protectedBar() { return bar(); }
>>>>> 
>>>>>  RefPtr<Bar> m_bar;
>>>>> }
>>>>> 
>>>>> Assuming there is no other code modifying m_bar, foo->bar()->method() is always safe to call even if method wasn’t a trivial function. Right now, static analyzer doesn’t recognize this pattern so we’d be forced to write code like this: foo->protectedBar()->method() where ensureProtectedBar is a wrapper around ensureBar which returns Ref<Bar>.
>>>>> 
>>>>> A suggestion was made that static analyzer can recognize this patten. Specifically, if we introduced a new smart pointer types that only allow setting the value once, static analyzer can allow foo->bar()->method()and avoid ref-churn in some cases:
>>>>> 
>>>>> e.g.
>>>>> 
>>>>> class Foo {
>>>>>  Bar& bar() {
>>>>>      if (!m_bar)
>>>>>          m_bar = Bar::create();
>>>>>      return *m_bar;
>>>>>  }
>>>>> 
>>>>>  SetOnceRefPtr<Bar> m_bar;
>>>>> }
>>>>> 
>>>>> SetOnceRefPtr::operator=(T*) release asserts that m_ptr isn’t set, and doesn’t have a move constructor, operator=(nullptr_t), leakRef(), releaseNonNull(), etc… which can override the value of m_ptr after setting the value via operator= or in constructor.
>>>>> 
>>>>> We could create various variants: SetOnceRef, SetOnceUniquePtr, SetOnceUniqueRef.
>>>>> 
>>>>> Do you think this will be useful? 
>>>>> 
>>>>> - R. Niwa
>>>> 
>>>> 
>>>> _______________________________________________
>>>> webkit-dev mailing list
>>>> webkit-dev at lists.webkit.org
>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>> 
>>> _______________________________________________
>>> webkit-dev mailing list
>>> webkit-dev at lists.webkit.org
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> 
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev at lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>