Fix linkage diagnostic so it doesn't ICE for external crates

[pnkfelix]

Fix linkage diagnostic so it doesn't ICE for external crates

(As a drive-by improvement, improved the diagnostic to indicate why *const T or *mut T is required.)

Fix #59548
Fix #61232

[rust-highfive]

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

[pnkfelix]

As I commented on the issue, I have a separate change drafted (but not yet posted to a PR) that adds a similar check earlier in the compiler pipeline, so that we would get this error at the definition site where the attribute occurs, rather that at the usage site where the item is used. (That is, AFAICT, when you add this attribute to an incompatibly-typed definition, there is no way to use it correctly, so we should consider erroring at the definition site instead.)

That would be a somewhat riskier change, since it would actually break existing code, unlike this PR, which is strictly keeping the compiler's behavior the same in non-ICE cases. But then again, this is an unstable feature anyway, so we don't have to be quite so cautious.

[Centril]

But then again, this is an unstable feature anyway, so we don't have to be quite so cautious.

I agree, I suspect the feature isn't so widely used anyways so I think it would be good to follow this up with your draft.

[pnkfelix]

But then again, this is an unstable feature anyway, so we don't have to be quite so cautious.

I agree, I suspect the feature isn't so widely used anyways so I think it would be good to follow this up with your draft.

The problem is that we would need to decide which variants of #[linkage="..."] to apply the check to.

(The way the code in consts.rs is written, one might infer that it applies to all variants of #[linkage="..."]; but other tests in our test suite seem to invalidate that inference.)

[pnkfelix]

But then again, this is an unstable feature anyway, so we don't have to be quite so cautious.

I agree, I suspect the feature isn't so widely used anyways so I think it would be good to follow this up with your draft.

The problem is that we would need to decide which variants of #[linkage="..."] to apply the check to.

One reason that answering this question may be harder than you expect is that back in PR #18890, we blindly added support for all variants of #[linkage="..."] without really trying to check whether they make any sense.

The comments for the code that parses the variant sort of hints at this:

rust/src/librustc_typeck/collect.rs

Lines 2485 to 2489 in ae8975c

	// Use the names from src/llvm/docs/LangRef.rst here. Most types are only
	// applicable to variable declarations and may not really make sense for
	// Rust code in the first place but whitelist them anyway and trust that
	// the user knows what s/he's doing. Who knows, unanticipated use cases
	// may pop up in the future.

but the point is, its pretty hard to go through and write tests for each case when some of them do not seem to make any sense for definitions in Rust code. Consider e.g. the appending case from LLVM:

“appending” linkage may only be applied to global variables of pointer to array type. When two global variables with appending linkage are linked together, the two global arrays are appended together. This is the LLVM, typesafe, equivalent of having the system linker append together “sections” with identical names when .o files are linked.

Unfortunately this doesn’t correspond to any feature in .o files, so it can only be used for variables like llvm.global_ctors which llvm interprets specially.

[pnkfelix]

Another reason why its tricky to "just" go through and try writing tests for each #[linkage="..."]: we basically can't put it on non-extern statics, because the existing check forces all uses of #[linkage="..."] to be a raw ptr (*const T or *mut T), but Rust itself forces definitions of non-extern statics to be Sync, and raw ptrs are never Sync.

(This may just be adding fuel to the argument that I should not be worrying about adding the check at the definition site. I am going to switch to looking at the cases that fail in our test suite when that definition site check is added, as that will help me understand what cases we supposedly support now.)

---

Update: This claim, that all #[linkage] cases must be raw ptr type, is apparently untrue. Or at least, I see a counter-example in run-make-fulldeps/linkage-attr-on-static

#[no_mangle]
#[linkage = "external"]
static BAZ: i32 = 21;

// ...

fn main() {
    unsafe {
       assert_eq!(what(), BAZ);
    }
}

I find all of this quite weird since in my own experiments it seemed like raw-ptrs were always required (even though that seemed like a generally unworkable restriction). Looking some more into this.

---

Update 2: Okay, I had misread the code earlier. The current compiler doesn't require all uses of #[linkage] to have raw-ptr type. The cde in linkage-attr-on-static is being very careful to define the item locally and then only import it from C code, not Rust code.

The check does require #[linkage] to have raw-ptr type in two cases:

• extern statics (as in extern { #[linkage="..."] pub static FOO = ...; })
• item defined in (and imported from) external crates.

That second bullet is a reason why its hard to do this check at the definition site; unless I make the check fire based on visibility (that is, any visibility strictly looser than pub(crate) would cause the check to fire), I don't know how best to determine the intent of a given item's definition.

(And it seems more likely to me that the check within our codegen is broken, and not many people are complaining because the feature itself is unstable and buggy?)

[petrochenkov]

@bors r+

[bors]

📌 Commit c8887ab has been approved by petrochenkov