Unicode check for crate_name attribute #2463

tamaroning · 2023-07-21T04:14:43Z

depends on PR #2425
Addresses #2287

Modify the function validate_crate_name to check if the crate name contains only Unicode alphabetic and numeric.
See https://doc.rust-lang.org/reference/crates-and-source-files.html#the-crate_name-attribute

gcc/rust/ChangeLog:

	* lex/rust-codepoint.h: Add comment
	* lex/rust-lex.h: New method to get decoded characters
	* rust-session-manager.cc (validate_crate_name): Modify unicode check
	(rust_crate_name_validation_test): Add testcases
	* util/rust-unicode.h (RUST_UNICODE_H): New class Utf8String.
	(class Utf8String): New class.

gcc/testsuite/ChangeLog:

	* rust/compile/bad-crate-name.rs: Moved to...
	* rust/compile/bad-crate-name1.rs: ...here.
	* rust/compile/bad-crate-name2.rs: New test.

Signed-off-by: Raiki Tamura <tamaron1203@gmail.com>

tamaroning · 2023-07-21T04:28:07Z

gcc/rust/lex/rust-codepoint.h

+// FIXME: move this to rust-unicode.h?
 struct Codepoint


I think this class should be moved to rust-unicode.h.
At least it should not be put in under gcc/rust/lex/.

that seems good to me

tamaroning · 2023-07-21T04:32:11Z

gcc/rust/rust-session-manager.cc

-  // FIXME: The next test does not pass as of current implementation
-  // ASSERT_TRUE (Rust::CompileOptions::validate_crate_name ("惊吓"));


this original comment is probably wrong.
The kanji 惊(U+60CA) are not categorized as Alphabetic.

In https://www.unicode.org/Public/14.0.0/ucd/DerivedCoreProperties.txt

... 30FF ; Alphabetic # Lo KATAKANA DIGRAPH KOTO 3105..312F ; Alphabetic # Lo [43] BOPOMOFO LETTER B..BOPOMOFO LETTER NN 3131..318E ; Alphabetic # Lo [94] HANGUL LETTER KIYEOK..HANGUL LETTER ARAEAE 31A0..31BF ; Alphabetic # Lo [32] BOPOMOFO LETTER BU..BOPOMOFO LETTER AH 31F0..31FF ; Alphabetic # Lo [16] KATAKANA LETTER SMALL KU..KATAKANA LETTER SMALL RO 3400..4DBF ; Alphabetic # Lo [6592] CJK UNIFIED IDEOGRAPH-3400..CJK UNIFIED IDEOGRAPH-4DBF 4E00..A014 ; Alphabetic # Lo [21013] CJK UNIFIED IDEOGRAPH-4E00..YI SYLLABLE E A015 ; Alphabetic # Lm YI SYLLABLE WU A016..A48C ; Alphabetic # Lo [1143] YI SYLLABLE BIT..YI SYLLABLE YYR A4D0..A4F7 ; Alphabetic # Lo [40] LISU LETTER BA..LISU LETTER OE ...

thanks for checking haha. did you try it with rustc directly?

oh my god. i just checked and rustc compiles it (@_@)

I overlooked 4E00..A014 containing 0x60CA haha

hahaha no worries it's all good :D

tamaroning · 2023-07-23T08:22:09Z

The new introduced class Utf8String can be used to fix #2411

CohenArthur

Looks good, thank you! Good work!

CohenArthur · 2023-07-27T09:19:22Z

gcc/rust/lex/rust-codepoint.h

+// FIXME: move this to rust-unicode.h?
 struct Codepoint


that seems good to me

CohenArthur · 2023-07-27T09:20:27Z

gcc/rust/rust-session-manager.cc

+      error = Error (UNDEF_LOCATION, "crate name is not a valid UTF-8 string");
+      return false;
+    }
+  if (uchars.value ().empty ())


maybe it would be clearer to add a temporary variable after the first if (!uchars.has_value())? You keep calling uchars.value() which maybe hurts readability just a little bit?

Sounds good. fixed

CohenArthur · 2023-07-27T09:20:38Z

gcc/rust/rust-session-manager.cc

@@ -113,24 +114,32 @@ infer_crate_name (const std::string &filename)
 static bool
 validate_crate_name (const std::string &crate_name, Error &error)
 {
-  if (crate_name.empty ())
+  Utf8String utf8_name = {crate_name};
+  auto uchars = utf8_name.get_chars ();


So I would rename this variable to uchars_opt

CohenArthur · 2023-07-27T09:21:15Z

gcc/rust/rust-session-manager.cc

+    {
+      error = Error (UNDEF_LOCATION, "crate name is not a valid UTF-8 string");
+      return false;
+    }


and then add this

Suggested change

}

}

auto uchars = uchars_opt.value();

CohenArthur · 2023-07-27T09:21:39Z

gcc/rust/rust-session-manager.cc

-  // FIXME: The next test does not pass as of current implementation
-  // ASSERT_TRUE (Rust::CompileOptions::validate_crate_name ("惊吓"));


thanks for checking haha. did you try it with rustc directly?

CohenArthur · 2023-07-27T09:22:34Z

gcc/rust/rust-session-manager.cc

 static bool
 validate_crate_name (const std::string &crate_name, Error &error)


this function would probably benefit from the new tl::expected type we have, but that's a problem for another PR!

philberty · 2023-07-29T23:01:49Z

gcc/rust/util/rust-unicode.h


 namespace Rust {

+class Utf8String
+{
+private:


I was about to say a nit here to move the private fields to the bottom of the class declaration but then i realised we do it this style in AST and HIR classes.

What is the GCC style? I think having them at the top like this is probably the best now when i think about it.

@dkm @CohenArthur @tschwinge any opinions on the style of putting private fields at the top of the class or bottom?

I like having private fields at the bottom of my classes personally. I never gave it much attention haha

philberty

LGTM

gcc/rust/ChangeLog: * lex/rust-codepoint.h: Add comment * lex/rust-lex.h: New method to get decoded characters * rust-session-manager.cc (validate_crate_name): Modify unicode check (rust_crate_name_validation_test): Add testcases * util/rust-unicode.h (RUST_UNICODE_H): New class Utf8String. (class Utf8String): New class. * util/rust-unicode.cc (binary_search_sorted_array): Add comment. (recursive_decomp_cano): Add comment. (recomp): Remove dead code. (dump_string): Removed. gcc/testsuite/ChangeLog: * rust/compile/bad-crate-name.rs: Moved to... * rust/compile/bad-crate-name1.rs: ...here. * rust/compile/bad-crate-name2.rs: New test. Signed-off-by: Raiki Tamura <tamaron1203@gmail.com>

tamaroning · 2023-07-30T09:01:15Z

Thank you for review.
Rebased and fixed parts @CohenArthur pointed out.

philberty

LGTM

tamaroning changed the title ~~[wip}~~ [wip] unicode check for crate_name attribute Jul 21, 2023

tamaroning force-pushed the crate-name branch from 3c71259 to bacbba9 Compare July 21, 2023 04:22

tamaroning changed the title ~~[wip] unicode check for crate_name attribute~~ Unicode check for crate_name attribute Jul 21, 2023

tamaroning commented Jul 21, 2023

View reviewed changes

tamaroning marked this pull request as ready for review July 21, 2023 04:33

This was referenced Jul 21, 2023

Unicode support #2287

Open

Add header file generator for Unicode normalization and alphanumeric check #2425

Merged

tamaroning force-pushed the crate-name branch 3 times, most recently from 7d8619a to 3631430 Compare July 21, 2023 05:20

CohenArthur requested review from CohenArthur and philberty and removed request for CohenArthur July 27, 2023 09:18

CohenArthur added the enhancement label Jul 27, 2023

CohenArthur approved these changes Jul 27, 2023

View reviewed changes

philberty reviewed Jul 29, 2023

View reviewed changes

philberty approved these changes Jul 29, 2023

View reviewed changes

philberty added this to the GCC 14 Stage 3 milestone Jul 29, 2023

tamaroning force-pushed the crate-name branch from 3631430 to 0db8921 Compare July 30, 2023 08:25

tamaroning force-pushed the crate-name branch from 0db8921 to 467dc47 Compare July 30, 2023 08:49

tamaroning mentioned this pull request Jul 30, 2023

Normalize Hangul #2467

Merged

philberty approved these changes Jul 30, 2023

View reviewed changes

philberty added this pull request to the merge queue Jul 30, 2023

Merged via the queue into Rust-GCC:master with commit a3fc40c Jul 30, 2023
9 checks passed

tamaroning deleted the crate-name branch July 31, 2023 01:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unicode check for crate_name attribute #2463

Unicode check for crate_name attribute #2463

tamaroning commented Jul 21, 2023 •

edited

Loading

tamaroning Jul 21, 2023

CohenArthur Jul 27, 2023

tamaroning Jul 21, 2023

CohenArthur Jul 27, 2023

tamaroning Jul 30, 2023 •

edited

Loading

tamaroning Jul 30, 2023

CohenArthur Aug 1, 2023

tamaroning commented Jul 23, 2023

CohenArthur left a comment

CohenArthur Jul 27, 2023

CohenArthur Jul 27, 2023

tamaroning Jul 30, 2023

CohenArthur Jul 27, 2023

CohenArthur Jul 27, 2023

CohenArthur Jul 27, 2023

CohenArthur Jul 27, 2023

philberty Jul 29, 2023

CohenArthur Aug 1, 2023

philberty left a comment

tamaroning commented Jul 30, 2023

philberty left a comment

		// FIXME: The next test does not pass as of current implementation
		// ASSERT_TRUE (Rust::CompileOptions::validate_crate_name ("惊吓"));

		static bool
		validate_crate_name (const std::string &crate_name, Error &error)

Unicode check for crate_name attribute #2463

Unicode check for crate_name attribute #2463

Conversation

tamaroning commented Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamaroning Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamaroning commented Jul 23, 2023

CohenArthur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philberty left a comment

Choose a reason for hiding this comment

tamaroning commented Jul 30, 2023

philberty left a comment

Choose a reason for hiding this comment

tamaroning commented Jul 21, 2023 •

edited

Loading

tamaroning Jul 30, 2023 •

edited

Loading