How should duplicate attributes be handled?
- Dominant language
- LLVM
- Stars
- 1.5k
- Forks
- 854
- Avg merge
- 3d 17h
- Merged PRs (30d)
- 137
Description
We're inconsistent with how we report duplicate attributes and I'm wondering what direction we'd like to resolve the inconsistencies.
Some attributes check to see if the same attribute was already applied to the declaration, but do not check whether the arguments to the attributes are the same. We then issue a diagnostic saying that the attribute is applied with different parameters (which may be wrong!). e.g., https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L3194
Some attributes check the attribute argument values to see if they're different between the two attributes and only diagnose if they're different. e.g., https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L3180
Still others use a helper method that we introduced called `checkForDuplicateAttribute()`, which reports any kind of duplication regardless of arguments. We use this both for attributes that have no arguments (https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L5597) and attributes that have arguments (https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L5621).
In community, the usual approach is to only diagnose a duplicate attribute if the arguments are different between the attributes, and to warn + retain (https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaDeclAttr.cpp#L2980) or to warn + drop the new attribute (https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaDeclAttr.cpp#L3026, https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaDeclAttr.cpp#L3227).
Given that we seem to almost uniformly warn + retain (except for https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L3295), I think the correct fix here is to not warn about duplicate attributes that have no arguments or when the attribute arguments are identical (which means we'll silently accumulate the duplicates in the AST). For attributes whose arguments differ, it may make sense to continue to diagnose the confusion, but should we drop one attribute or the other (or both)?
Contributor guide
Assessment
This issue has not been assessed yet.