It's a good idea but I think it is out of the scope of this patch, since the same logic is already duplicated in eevee twice? If you like I can make an independent cleanup patch after this and D15941.

I think this is complicating it beyond need - this is specifically for shader attribute nodes, which essentially always output RGBA due to the output sockets they have. Compact storage is worthless at least unless renderers are refactored to treat instances in a coalesced manner, i.e. get them from the dupli system as grouped arrays rather than looping over individual instances, because currently per-instance metadata occupies same or more space than the data.

Maybe the function could be called find_color_attribute to reflect its fixed output type?

Wouldn't expect you to have found this, the multiple attribute APIs make this confusing. But this can be even simpler:

const bke::AttributeAccessor attributes = component->attributes();
const VArray data = attributes.lookup<ColorGeometry4f>(name);
copy_v4_v4(r_value, data[dupli->instance_idx[i]];

Didn't compile that to test, but the big picture should work. Looking up with a specific data type means the attribute API handles the conversions for you. It will only convert the value for the single index you request.

I simplified it more, but note that:

• I don't want any 'builtin' attributes to be included in the interest of reducing user surprise.
• Lookup should fail hard if the attribute doesn't exist or the type is incompatible and can't be converted, no 'defaults' allowed.

I'm not sure how your code fares on this front.

I don't want any 'builtin' attributes to be included in the interest of reducing user surprise.

The attribute API should be consistent between editors. If you can retrieve a builtin attribute in geometry nodes or in the attribute list in the UI, it should also be possible to retrieve it in shaders. The only builtin attribute for instances is "position" anyway.

Lookup should fail hard if the attribute doesn't exist or the type is incompatible and can't be converted, no 'defaults' allowed.

lookup<T>() returns an empty virtual array that will be "false" if the attribute doesn't exist.

Why does it use 4 here and MAX_DUPLI_RECUR in DupliContext?

Use geometry instead of gset. Same below everywhere.

instance_index

Follow code style for type casts.
https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast

You might have to iterate over the instances in reverse so that "inner" instances have higher priority.

It is trying to save space in DupliObject due to concerns by only storing non-null entries here, while DupliContext stores all entries. This basically bets half of entries in the stack won't be bound to geometry node instances (e.g. technical entries for persistent id numbering).

The list in DupliObject is already reversed.

Ok, I think it would be good to mention that in a comment. And also mention the it in the patch/commit description. Hopefully we can get rid of these limits soonish (want to work on this at some point), but until then it would be nice if the limit is more obvious.

Ah ok, please mention that in a comment, just to make the intend clear.

This requires less thinking about the order of things, and easier changes if this code is ever refactored later.