No unused attribute warning for attributes without "bs." prefix

[cknitt]

I came across this while cleaning up the bs. prefix handling.

@bs.string
let x = 42

gives

[E] Line 1, column 0:
Unused attribute: bs.string
This means such annotation is not annotated properly. 
for example, some annotations is only meaningful in externals 

whereas

@string
let x = 42

gives no error.

[cknitt]

Actually this is not completely done yet.

See https://github.com/rescript-lang/rescript-compiler/blob/1bc9e034a11707f0fff6646b66a0a15f716eeab5/jscomp/frontend/bs_ast_invariant.ml#L31

I have not activated the check for @as and @int as it gives some unexpected errors.

[cknitt]

As stated above, the unused unused attribute checks are currently still disabled for @as and @int.

I now looked into @as again. It turns out this is currently not marked as used for untagged variants.
I think what would need to be done is to call Ast_attributes.iter_process_bs_string_as in Ast_untagged_variants.process_tag_type.

However, this won't work as the former is in frontend whereas the latter is in ml (and frontend depends on ml, not vice versa). Moving ast_untagged_variants.ml from ml to frontend didn't work for me either (lots of errors).

How to best resolve this? @cristianoc

[cristianoc]

As stated above, the unused unused attribute checks are currently still disabled for @as and @int.

I now looked into @as again. It turns out this is currently not marked as used for untagged variants. I think what would need to be done is to call Ast_attributes.iter_process_bs_string_as in Ast_untagged_variants.process_tag_type.

However, this won't work as the former is in frontend whereas the latter is in ml (and frontend depends on ml, not vice versa). Moving ast_untagged_variants.ml from ml to frontend didn't work for me either (lots of errors).

How to best resolve this? @cristianoc

One possibility is to add a callback into Ast_untagged_variants:

let mark_used_bs_attribute_ref : (Parsetree.attribute -> unit) ref = ref (fun _ -> ())

Then set it in Bs_ast_invariant:

let mark_used_bs_attribute ((x, _) : Parsetree.attribute) =
  if not x.loc.loc_ghost then Hash_set_poly.add used_attributes x

let () = Ast_untagged_variants.mark_used_bs_attribute_ref := mark_used_bs_attribute

That said it's not completely clear what future uses of as could be, or in a separate ppx.
E.g. there are talks of adding @as(...) type t = to gentype.

[cknitt]

That said it's not completely clear what future uses of as could be, or in a separate ppx.
E.g. there are talks of adding @as(...) type t = to gentype.

So are you in favor of fixing this? Personally I find the unused attribute warnings quite helpful.
If additional use cases for @as come up in gentype, we can mark those as used, too.

3rd party PPXs should use different/"namespaced" attribute names anyway IMHO.

[cristianoc]

Sure it can be fixed.
Here's a better refactoring that does not require using callbacks or other tricks: #6795

[cknitt]

@cristianoc Thanks a lot! This solution is much better than the callback trick! 🎉

[cristianoc]

@cknitt moved the check earlier: into the invariant check code.

[cknitt]

Ok, so after #6795 and #6802, @as and @int are done, and the list of attributes that are checked is

  | "as" | "bs" | "config" | "ignore" | "inline" | "int" | "optional" | "string"
  | "uncurry" | "unwrap"

However, looking at https://rescript-lang.org/syntax-lookup, there are many more attributes. For which of these would it make sense to add checks?

For example, what about @inline? This is something that OCaml 5.2 started checking BTW (inline allowed in .ml only, not in .mli).