Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ambiguous type deduction around file imports (frontend 2.110) #20635

Closed
CromFr opened this issue Jan 4, 2025 · 5 comments · Fixed by #20679
Closed

Ambiguous type deduction around file imports (frontend 2.110) #20635

CromFr opened this issue Jan 4, 2025 · 5 comments · Fixed by #20679
Labels
Severity:Regression PRs that fix regressions

Comments

@CromFr
Copy link

CromFr commented Jan 4, 2025

When importing a file as data, the type get ambiguously detected and leads to strange type errors, hinting that a ubyte[] could get implicitly converted to string.

This bug appeared on both LDC 1.40.0 and DMD 2.110.0-beta.1, and seems to be related with the frontend version 2110.

LDC 1.39.0 and DMD v2.109.1 are fine.

This issue was originally opened on LDC: ldc-developers/ldc#4813

Reproduction

  1. Create a new dub package, and edit dub.json to add "stringImportPaths": ["."]
  2. Copy this to source/app.d:
void fn(in ubyte[] value){}
void fn(in string value){}

void main() {
	// Error: `app.fn` called with argument types `(ubyte[])` matches both:
	//     `app.fn(in ubyte[] value)`
	// and:
	//     `app.fn(in string value)`
	fn(cast(ubyte[])import("dub.json"));

	// Ok
	fn(cast(immutable ubyte[])[]);

	// Ok
	string valuestr = "hello world";
	fn(cast(ubyte[])valuestr);
}
  1. Run dub run

The full output is:

$ dub run --compiler=dmd
    Starting Performing "debug" build using dmd for x86_64.
    Building dubtest ~master: building configuration [application]
source/app.d(9,4): Error: `app.fn` called with argument types `(ubyte[])` matches both:
source/app.d(1,6):     `app.fn(in ubyte[] value)`
and:
source/app.d(2,6):     `app.fn(in string value)`
Error dmd failed with exit code 1.

I couldn't reproduce the issue without the import. Importing a text or a binary file gives the same error.

@thewilsonator thewilsonator added the Severity:Regression PRs that fix regressions label Jan 4, 2025
@thewilsonator
Copy link
Contributor

cc @Geod24

@thewilsonator thewilsonator changed the title Ambiguous type deduction around file imports (frontend 2110) Ambiguous type deduction around file imports (frontend 2.110) Jan 4, 2025
@Herringway
Copy link
Contributor

Regression was brought to the surface by #16638, possibly caused by #16263.

#16638 caused string imports to be treated as hex strings. Note that they suffer from the same issue:

void fn(in ubyte[] value){}
void fn(in string value){}

void main() {
	// Error: `app.fn` called with argument types `(ubyte[])` matches both:
	//     `app.fn(in ubyte[] value)`
	// and:
	//     `app.fn(in string value)`
	fn(cast(ubyte[])x"AA BB CC DD");

	// Ok
	fn(cast(immutable ubyte[])[]);

	// Ok
	string valuestr = "hello world";
	fn(cast(ubyte[])valuestr);
}

While I haven't done the bisection to confirm it, this version of the code regressed with DMD 2.108.0, which happened to include #16263, which dramatically changed how these work.

ping @dkorpel

@dkorpel
Copy link
Contributor

dkorpel commented Jan 4, 2025

There's a lot going on.

First of all, immutable data is being cast to a mutable ubyte[]. That's bad practice, but I don't think it invalidates the issue.

Then, without -preview=in, the two overloads are equivalent to:

void fn(const ubyte[] value){}
void fn(const string value){}

Because the cast is to ubyte[] and not const ubyte[], the first function isn't an exact match.

The second function used to be 'no match', because the cast(ubyte[]) would be done at run time. But now, file imports are now treated as hex strings, which convert to ubyte[] at compile time, and there's a quirk:

strange type errors, hinting that a ubyte[] could get implicitly converted to string

Compile-time known integer arrays do convert to strings, even after explicitly casting:

string x = [1, 2, 3];
string y = cast(short[]) [4, 5, 6];

So now both overloads match with implicit conversions, so you get an ambiguity error.

I'm not sure why the first overload doesn't match with 'qualifier conversion' (which has priority over 'implicit conversion'), but I'll look into it.

Also I hate how cast() is used both for re-interpret casting and element-casting array elements depending on whether it's compile-time or run-time, but that's how it is and existing code relies on it.

In terms of solutions, I'm thinking:

  • See why there's no qualifier conversion match on the first overload
  • Don't make compile-time integer arrays convert to strings, at least after explicitly type casting (could be breaking)
  • Make hex strings unable to convert to string after casting to an integer array (less breaking, but more special case-y)
  • Put the hex-string file import change behind a next edition (it also broke Herringway's code by making certain casts big-endian instead of target-endian, Treat import("file") as hex string #16638 (comment))

@CromFr
Copy link
Author

CromFr commented Jan 6, 2025

First of all, immutable data is being cast to a mutable ubyte[]. That's bad practice, but I don't think it invalidates the issue.

FYI the original code had cast(immutable ubyte[])import(...), but I removed it to reduce the amount of information in the error message. The behaviour is the same with or without the immutability.

@dkorpel
Copy link
Contributor

dkorpel commented Jan 10, 2025

It turns out a bug made the match level of qualifier conversion get overridden by implicit conversion. Fixed by #20679, I added a test for both the mutable and immutable cast.

@dkorpel dkorpel closed this as completed Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Severity:Regression PRs that fix regressions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants